[PATCH] Fix ssa coalescing with inline asm (PR middle-end/70593)

Richard Biener rguenther@suse.de
Fri Apr 8 16:04:00 GMT 2016


On April 8, 2016 4:54:22 PM GMT+02:00, Jakub Jelinek <jakub@redhat.com> wrote:
>Hi!
>
>xen is miscompiled since Alex' SSA coalescing changes.
>The problem is that we have there inline asm that sets more than one
>SSA_NAME, one of them is dead though (has zero uses) and because of
>the zero uses coalescing doesn't see any conflicts and puts both
>the SSA_NAMEs in the two GIMPLE_ASM outputs into the same partition.
>During expansion we then emit the ASM_OPERANDS followed by copying of
>the
>two outputs into DECL_RTL of their SSA_NAMEs.  But as both are in the
>same
>partition, that is the same pseudo.  The first one is what we want,
>and the second one is for the zero uses SSA_NAME, which then overwrites
>the right one with unrelated value.
>(insn 9 6 7 2 (parallel [
>            (set (reg:DI 89 [ c ])
>                (asm_operands/v:DI ("xorl	%k1, %k1
>	movl	$7, %k0") ("=c") 0 [
>                        (reg/v:DI 87 [ <retval> ])
>                        (reg/v:DI 87 [ <retval> ])
>                    ]
>                     [
>                        (asm_input:DI ("0") pr70593.c:9)
>                        (asm_input:DI ("1") pr70593.c:9)
>                    ]
>                     [] pr70593.c:9))
>            (set (reg:DI 90 [ a ])
>                (asm_operands/v:DI ("xorl	%k1, %k1
>	movl	$7, %k0") ("=a") 1 [
>                        (reg/v:DI 87 [ <retval> ])
>                        (reg/v:DI 87 [ <retval> ])
>                    ]
>                     [
>                        (asm_input:DI ("0") pr70593.c:9)
>                        (asm_input:DI ("1") pr70593.c:9)
>                    ]
>                     [] pr70593.c:9))
>            (clobber (mem:BLK (scratch) [0  A8]))
>            (clobber (reg:CCFP 18 fpsr))
>            (clobber (reg:CC 17 flags))
>        ]) pr70593.c:9 -1
>     (nil))
>(insn 7 9 8 2 (set (reg/v:DI 87 [ <retval> ])
>        (reg:DI 89 [ c ])) pr70593.c:9 -1
>     (nil))
>(insn 8 7 13 2 (set (reg/v:DI 87 [ <retval> ])
>        (reg:DI 90 [ a ])) pr70593.c:9 -1
>     (nil))
>The following patch handles this by making sure we record conflicts
>between multiple SSA_NAME outputs of the GIMPLE_ASM, even when some of
>them
>are not live, by emulating what really happens during expansion - that
>they are live in the moves after the asm insn.  The
>live_track_process_def calls later on remove it again from the live
>partitions and thus undo the live_track_process_use effect, so all
>the patch changes is that during the processing of e.g. the first
>SSA_NAME output it sees the second (and perhaps third ...) SSA_NAME
>output
>as live and adds conflict, similarly when processing the second one
>if there are more than two, it again sees the third one and adds
>conflict
>etc.
>
>Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

Hmm, don't we simply want to do this for all stmts (OK, only asm have multiple defs...)?

Otherwise OK.thanks,
Richard.

>2016-04-08  Jakub Jelinek  <jakub@redhat.com>
>
>	PR middle-end/70593
>	* tree-ssa-coalesce.c (build_ssa_conflict_graph): For GIMPLE_ASM
>	with multiple SSA_NAME defs, force the outputs other than first
>	to be live before calling live_track_process_def on each output.
>
>	* gcc.target/i386/pr70593.c: New test.
>
>--- gcc/tree-ssa-coalesce.c.jj	2016-03-30 16:00:17.000000000 +0200
>+++ gcc/tree-ssa-coalesce.c	2016-04-08 12:36:26.409403204 +0200
>@@ -905,6 +905,27 @@ build_ssa_conflict_graph (tree_live_info
> 	    }
> 	  else if (is_gimple_debug (stmt))
> 	    continue;
>+	  else if (gimple_code (stmt) == GIMPLE_ASM
>+		   && gimple_asm_noutputs (as_a <gasm *> (stmt)) > 1)
>+	    {
>+	      /* For GIMPLE_ASM as the only statement which can have
>+		 more than one SSA_NAME definition, pretend all the
>+		 SSA_NAME outputs but the first one are live at this point,
>+		 so that conflicts are added in between all those even
>+		 when they are actually not really live after the asm,
>+		 because expansion might copy those into pseudos after
>+		 the asm and if multiple outputs share the same partition,
>+		 it might overwrite those that should be live.  E.g.
>+		 asm volatile (".." : "=r" (a) : "=r" (b) : "0" (a), "1" (a));
>+		 return a;
>+		 See PR70593.  */
>+	      bool first = true;
>+	      FOR_EACH_SSA_TREE_OPERAND (var, stmt, iter, SSA_OP_DEF)
>+		if (first)
>+		  first = false;
>+		else
>+		  live_track_process_use (live, var);
>+	    }
> 
> 	  FOR_EACH_SSA_TREE_OPERAND (var, stmt, iter, SSA_OP_DEF)
> 	    live_track_process_def (live, var, graph);
>--- gcc/testsuite/gcc.target/i386/pr70593.c.jj	2016-04-08
>12:59:27.352278471 +0200
>+++ gcc/testsuite/gcc.target/i386/pr70593.c	2016-04-08
>12:48:27.000000000 +0200
>@@ -0,0 +1,19 @@
>+/* PR middle-end/70593 */
>+/* { dg-do run } */
>+/* { dg-options "-O2" } */
>+
>+__attribute__((noinline, noclone)) unsigned long
>+foo (unsigned x)
>+{
>+  unsigned long a, c = x;
>+  asm volatile ("xorl\t%k1, %k1\n\tmovl\t$7, %k0" : "=c" (c), "=a" (a)
>: "0" (c), "1" (c) : "memory");
>+  return c;
>+}
>+
>+int
>+main ()
>+{
>+  if (foo (3) != 7)
>+    __builtin_abort ();
>+  return 0;
>+}
>
>	Jakub




More information about the Gcc-patches mailing list