diff mbox

[i386] : Fix PR/46219 Generate indirect jump instruction

Message ID 1705189073.20810228.1401825417250.JavaMail.zimbra@redhat.com
State New
Headers show

Commit Message

Kai Tietz June 3, 2014, 7:56 p.m. UTC
Hello,

This patch fixes PR/46219 by introducing special peephole-optimization.  As we can't set for new statement in peephole2-define SIBLING_CALL_P easily, I use UNSPEC_PEEPSIB to do indentify sibling tail-call-case.

For avoiding modification of ix86_output_call_insn, I set SIBLING_CALL_P directly before outputing it.  If it is preferred we can modify here instead ix86_output_call_insn to allow forcing to output sibcall on demand.

ChangeLog

2014-06-03  Kai Tietz  <ktietz@redhat.com>

	PR target/46219
	* config/i386/i386.md (UNSPEC_PEEPSIB): New unspec.
	(sibcall_intern): New define-insn to handle UNSPEC_PEEPSIB.
	(sibcall_pop_intern): Likewise.
	(sibcall_value_intern): Likewise.
	(sibcall_value_pop_intern): Likewise.
	(define_peephole2): Simple combine for sibling-tail-call.


ChangeLog

2014-06-03  Kai Tietz  <ktietz@redhat.com>

	PR target/46219
	* gcc.target/i386/sibcall-4.c: Remove xfail.

Tested patch for x86_64-unknown-linux-gnu, and i686-pc-cygwin.  Ok for apply?

Regards,
Kai

Comments

Richard Henderson June 3, 2014, 8:06 p.m. UTC | #1
On 06/03/2014 12:56 PM, Kai Tietz wrote:
> +(define_insn "*sibcall_intern"
> +  [(call (unspec [(mem:QI (match_operand:W 0 "memory_operand"))] UNSPEC_PEEPSIB)
> +	 (match_operand 1))]
> +  ""
> +  "* SIBLING_CALL_P (insn) = 1; return ix86_output_call_insn (insn, operands[0]);"
> +  [(set_attr "type" "call")])

Why would this be hard to do when first emitting it?

> +; TODO
> +(define_peephole2
> +  [(set (match_operand:DI 0 "register_operand")
> +        (match_operand:DI 1 "memory_operand"))
> +   (unspec_volatile [(const_int 0)] UNSPECV_BLOCKAGE)]
> +  "TARGET_64BIT"
> +  [(unspec_volatile [(const_int 0)] UNSPECV_BLOCKAGE)
> +   (set (match_dup 0)
> +        (match_dup 1))])
> +
> +(define_peephole2
> +  [(set (match_operand:SI 0 "register_operand")
> +        (match_operand:SI 1 "memory_operand"))
> +   (unspec_volatile [(const_int 0)] UNSPECV_BLOCKAGE)]
> +  "!TARGET_64BIT"
> +  [(unspec_volatile [(const_int 0)] UNSPECV_BLOCKAGE)
> +   (set (match_dup 0)
> +        (match_dup 1))])

These are wrong.  This allows unrestricted movement across the blockage.

> +(define_peephole2
> +  [(set (match_operand:DI 0 "register_operand") 
> +        (match_operand:DI 1 "memory_operand"))
> +   (call (mem:QI (match_operand:DI 2 "register_operand"))
> +         (match_operand 3))]
> +  "TARGET_64BIT  && REG_P (operands[0])
> +    && REG_P (operands[2])
> +    && SIBLING_CALL_P (peep2_next_insn (1))
> +    && REGNO (operands[0]) == REGNO (operands[2])"
> +  [(call (unspec [(mem:QI (match_dup 1))] UNSPEC_PEEPSIB) (match_dup 3))])

What you wanted was to match 3 insns in a peephole like this, including the
blockage, and including re-emitting the blockage in the output.


r~
Kai Tietz June 3, 2014, 8:15 p.m. UTC | #2
----- Original Message -----
> On 06/03/2014 12:56 PM, Kai Tietz wrote:
> > +(define_insn "*sibcall_intern"
> > +  [(call (unspec [(mem:QI (match_operand:W 0 "memory_operand"))]
> > UNSPEC_PEEPSIB)
> > +	 (match_operand 1))]
> > +  ""
> > +  "* SIBLING_CALL_P (insn) = 1; return ix86_output_call_insn (insn,
> > operands[0]);"
> > +  [(set_attr "type" "call")])
> 
> Why would this be hard to do when first emitting it?

To set this flag on the peephole causes some hazard with unknown-instruction, as the memory isn't part of the existing SIBLING_CALL_P (insn) rules.  And of course we don't want to allow general memory-operand for sibling-calls, so it is hard (and useless) to set it in peephole.  Additionally it would mean to replace insn-generation generation by hand-written block to get access to the insn (code generation adds added { ... } block to the beginning of the peephole-generation routine).
 
> > +; TODO
> > +(define_peephole2
> > +  [(set (match_operand:DI 0 "register_operand")
> > +        (match_operand:DI 1 "memory_operand"))
> > +   (unspec_volatile [(const_int 0)] UNSPECV_BLOCKAGE)]
> > +  "TARGET_64BIT"
> > +  [(unspec_volatile [(const_int 0)] UNSPECV_BLOCKAGE)
> > +   (set (match_dup 0)
> > +        (match_dup 1))])
> > +
> > +(define_peephole2
> > +  [(set (match_operand:SI 0 "register_operand")
> > +        (match_operand:SI 1 "memory_operand"))
> > +   (unspec_volatile [(const_int 0)] UNSPECV_BLOCKAGE)]
> > +  "!TARGET_64BIT"
> > +  [(unspec_volatile [(const_int 0)] UNSPECV_BLOCKAGE)
> > +   (set (match_dup 0)
> > +        (match_dup 1))])
> 
> These are wrong.  This allows unrestricted movement across the blockage.

Well, I didn't noticed any serious regression by it.  Anyway I will add blockage-check to sibcall-pattern.
 
> > +(define_peephole2
> > +  [(set (match_operand:DI 0 "register_operand")
> > +        (match_operand:DI 1 "memory_operand"))
> > +   (call (mem:QI (match_operand:DI 2 "register_operand"))
> > +         (match_operand 3))]
> > +  "TARGET_64BIT  && REG_P (operands[0])
> > +    && REG_P (operands[2])
> > +    && SIBLING_CALL_P (peep2_next_insn (1))
> > +    && REGNO (operands[0]) == REGNO (operands[2])"
> > +  [(call (unspec [(mem:QI (match_dup 1))] UNSPEC_PEEPSIB) (match_dup 3))])
> 


Kai
Richard Henderson June 3, 2014, 8:36 p.m. UTC | #3
On 06/03/2014 01:15 PM, Kai Tietz wrote:
> ----- Original Message -----
>> On 06/03/2014 12:56 PM, Kai Tietz wrote:
>>> +(define_insn "*sibcall_intern"
>>> +  [(call (unspec [(mem:QI (match_operand:W 0 "memory_operand"))]
>>> UNSPEC_PEEPSIB)
>>> +	 (match_operand 1))]
>>> +  ""
>>> +  "* SIBLING_CALL_P (insn) = 1; return ix86_output_call_insn (insn,
>>> operands[0]);"
>>> +  [(set_attr "type" "call")])
>>
>> Why would this be hard to do when first emitting it?
> 
> To set this flag on the peephole causes some hazard with
> unknown-instruction,
as the memory isn't part of the existing SIBLING_CALL_P (insn) rules. And of
course we don't want to allow general memory-operand for sibling-calls, so it
is hard (and useless) to set it in peephole. Additionally it would mean to
replace insn-generation generation by hand-written block to get access to the
insn (code generation adds added { ... } block to the beginning of the
peephole-generation routine).


I think we need to get the SIBLING_CALL_P flag transferred immediately.
Otherwise we risk incorrect data flow analysis elsewhere.

We don't necessarily have to open-code this.  It looks like peep2_attempt
copies over some of the extra bits for calls, but not all of them.  Compare
this to what we have in try_split.


r~
diff mbox

Patch

Index: config/i386/i386.md
===================================================================
--- config/i386/i386.md	(revision 211198)
+++ config/i386/i386.md	(working copy)
@@ -111,6 +111,7 @@ 
   UNSPEC_LEA_ADDR
   UNSPEC_XBEGIN_ABORT
   UNSPEC_STOS
+  UNSPEC_PEEPSIB
 
   ;; For SSE/MMX support:
   UNSPEC_FIX_NOTRUNC
@@ -11382,6 +11383,54 @@ 
   "* return ix86_output_call_insn (insn, operands[0]);"
   [(set_attr "type" "call")])
 
+(define_insn "*sibcall_intern"
+  [(call (unspec [(mem:QI (match_operand:W 0 "memory_operand"))] UNSPEC_PEEPSIB)
+	 (match_operand 1))]
+  ""
+  "* SIBLING_CALL_P (insn) = 1; return ix86_output_call_insn (insn, operands[0]);"
+  [(set_attr "type" "call")])
+
+; TODO
+(define_peephole2
+  [(set (match_operand:DI 0 "register_operand")
+        (match_operand:DI 1 "memory_operand"))
+   (unspec_volatile [(const_int 0)] UNSPECV_BLOCKAGE)]
+  "TARGET_64BIT"
+  [(unspec_volatile [(const_int 0)] UNSPECV_BLOCKAGE)
+   (set (match_dup 0)
+        (match_dup 1))])
+
+(define_peephole2
+  [(set (match_operand:SI 0 "register_operand")
+        (match_operand:SI 1 "memory_operand"))
+   (unspec_volatile [(const_int 0)] UNSPECV_BLOCKAGE)]
+  "!TARGET_64BIT"
+  [(unspec_volatile [(const_int 0)] UNSPECV_BLOCKAGE)
+   (set (match_dup 0)
+        (match_dup 1))])
+
+(define_peephole2
+  [(set (match_operand:DI 0 "register_operand") 
+        (match_operand:DI 1 "memory_operand"))
+   (call (mem:QI (match_operand:DI 2 "register_operand"))
+         (match_operand 3))]
+  "TARGET_64BIT  && REG_P (operands[0])
+    && REG_P (operands[2])
+    && SIBLING_CALL_P (peep2_next_insn (1))
+    && REGNO (operands[0]) == REGNO (operands[2])"
+  [(call (unspec [(mem:QI (match_dup 1))] UNSPEC_PEEPSIB) (match_dup 3))])
+
+(define_peephole2
+  [(set (match_operand:SI 0 "register_operand") 
+        (match_operand:SI 1 "memory_operand"))
+   (call (mem:QI (match_operand:SI 2 "register_operand"))
+         (match_operand 3))]
+  "!TARGET_64BIT  && REG_P (operands[0])
+    && REG_P (operands[2])
+    && SIBLING_CALL_P (peep2_next_insn (1))
+    && REGNO (operands[0]) == REGNO (operands[2])"
+  [(call (unspec [(mem:QI (match_dup 1))] UNSPEC_PEEPSIB) (match_dup 3))])
+
 (define_expand "call_pop"
   [(parallel [(call (match_operand:QI 0)
 		    (match_operand:SI 1))
@@ -11415,6 +11464,16 @@ 
   "* return ix86_output_call_insn (insn, operands[0]);"
   [(set_attr "type" "call")])
 
+(define_insn "*sibcall_pop_intern"
+  [(call (unspec [(mem:QI (match_operand:SI 0 "memory_operand"))] UNSPEC_PEEPSIB)
+	 (match_operand 1))
+   (set (reg:SI SP_REG)
+	(plus:SI (reg:SI SP_REG)
+		 (match_operand:SI 2 "immediate_operand" "i")))]
+  "!TARGET_64BIT"
+  "* SIBLING_CALL_P (insn) = 1; return ix86_output_call_insn (insn, operands[0]);"
+  [(set_attr "type" "call")])
+
 ;; Call subroutine, returning value in operand 0
 
 (define_expand "call_value"
@@ -11457,6 +11516,14 @@ 
   "* return ix86_output_call_insn (insn, operands[1]);"
   [(set_attr "type" "callv")])
 
+(define_insn "*sibcall_value_intern"
+  [(set (match_operand 0)
+	(call (unspec [(mem:QI (match_operand:W 1 "memory_operand"))] UNSPEC_PEEPSIB)
+	      (match_operand 2)))]
+  ""
+  "* SIBLING_CALL_P (insn) = 1; return ix86_output_call_insn (insn, operands[1]);"
+  [(set_attr "type" "callv")])
+
 (define_insn "*call_value_rex64_ms_sysv"
   [(match_parallel 3 "call_rex64_ms_sysv_operation"
     [(set (match_operand 0)
@@ -11503,6 +11570,17 @@ 
   "* return ix86_output_call_insn (insn, operands[1]);"
   [(set_attr "type" "callv")])
 
+(define_insn "*sibcall_value_pop_intern"
+  [(set (match_operand 0)
+        (call (unspec [(mem:QI (match_operand:SI 1 "memory_operand"))] UNSPEC_PEEPSIB)
+	      (match_operand 2)))
+   (set (reg:SI SP_REG)
+	(plus:SI (reg:SI SP_REG)
+		 (match_operand:SI 3 "immediate_operand" "i")))]
+  "!TARGET_64BIT"
+  "* SIBLING_CALL_P (insn) = 1; return ix86_output_call_insn (insn, operands[1]);"
+  [(set_attr "type" "callv")])
+
 ;; Call subroutine returning any type.
 
 (define_expand "untyped_call"
Index: testsuite/gcc.target/i386/sibcall-4.c
===================================================================
--- testsuite/gcc.target/i386/sibcall-4.c	(revision 211198)
+++ testsuite/gcc.target/i386/sibcall-4.c	(working copy)
@@ -11,4 +11,4 @@  void male_indirect_jump (long offset)
   dispatch[offset](offset);
 }
 
-/* { dg-final { scan-assembler-not "jmp\[ \t\]*.%eax" { xfail *-*-* } } } */
+/* { dg-final { scan-assembler-not "jmp\[ \t\]*.%eax" } } */