PATCH: Properly check mode for x86 call/jmp address

On Wed, Mar 7, 2012 at 5:03 PM, H.J. Lu <hjl.tools@gmail.com> wrote:

>>>>>  (define_insn "*call"
>>>>> -  [(call (mem:QI (match_operand:P 0 "call_insn_operand" "<c>zw"))
>>>>> +  [(call (mem:QI (match_operand:C 0 "call_insn_operand" "<c>zw"))
>>>>>        (match_operand 1 "" ""))]
>>>>> -  "!SIBLING_CALL_P (insn)"
>>>>> +  "!SIBLING_CALL_P (insn)
>>>>> +   && (GET_CODE (operands[0]) == SYMBOL_REF
>>>>> +       || GET_MODE (operands[0]) == word_mode)"
>>>>
>>>> There are enough copies of this extra constraint that I wonder
>>>> if it simply ought to be folded into call_insn_operand.
>>>>
>>>> Which would need to be changed to define_special_predicate,
>>>> since you'd be doing your own mode checking.
>>>>
>>>> Probably similar changes to sibcall_insn_operand.
>>>
>>> Here is the updated patch.  I changed constant_call_address_operand
>>> and call_register_no_elim_operand to use define_special_predicate.
>>> OK for trunk?
>>
>> Please do not complicate matters that much. Just stick word_mode
>> overrides for register operands in predicates.md, like in attached
>> patch. These changed predicates now allow registers only in word_mode
>> (and VOIDmode).
>>
>> You can now remove all new mode iterators and leave call patterns untouched.
>>
>> @@ -22940,14 +22940,18 @@ ix86_expand_call (rtx retval, rtx fnaddr,
>> rtx callarg1,
>>       && GET_CODE (XEXP (fnaddr, 0)) == SYMBOL_REF
>>       && !local_symbolic_operand (XEXP (fnaddr, 0), VOIDmode))
>>     fnaddr = gen_rtx_MEM (QImode, construct_plt_address (XEXP (fnaddr, 0)));
>> -  else if (sibcall
>> -          ? !sibcall_insn_operand (XEXP (fnaddr, 0), Pmode)
>> -          : !call_insn_operand (XEXP (fnaddr, 0), Pmode))
>> +  else if (!(constant_call_address_operand (XEXP (fnaddr, 0), Pmode)
>> +            || call_register_no_elim_operand (XEXP (fnaddr, 0),
>> +                                              word_mode)
>> +            || (!sibcall
>> +                && !TARGET_X32
>> +                && memory_operand (XEXP (fnaddr, 0), word_mode))))
>>     {
>>       fnaddr = XEXP (fnaddr, 0);
>> -      if (GET_MODE (fnaddr) != Pmode)
>> -       fnaddr = convert_to_mode (Pmode, fnaddr, 1);
>> -      fnaddr = gen_rtx_MEM (QImode, copy_to_mode_reg (Pmode, fnaddr));
>> +      if (GET_MODE (fnaddr) != word_mode)
>> +       fnaddr = convert_to_mode (word_mode, fnaddr, 1);
>> +      fnaddr = gen_rtx_MEM (QImode,
>> +                           copy_to_mode_reg (word_mode, fnaddr));
>>     }
>>
>>   vec_len = 0;
>>
>> Please update the above part. It looks you don't even have to change
>> condition with new predicates. Basically, you should only convert the
>> address to word_mode instead of Pmode.
>>
>> +  if (TARGET_X32)
>> +    operands[0] = convert_memory_address (word_mode, operands[0]);
>>
>> This addition to indirect_jump and tablejump should be the only
>> change, needed in i386.md now. Please write the condition
>>
>> if (Pmode != word_mode)
>>
>> for consistency.
>>
>> BTW: The attached patch was bootstrapped and regression tested on
>> x86_64-pc-linux-gnu {,-m32}.
>>
>> Uros.
>
> It doesn't work:
>
> x.i:7:1: error: unrecognizable insn:
> (call_insn/j 8 7 9 3 (call (mem:QI (reg:DI 62) [0 *foo.0_1 S1 A8])
>        (const_int 0 [0])) x.i:6 -1
>     (nil)
>    (nil))
> x.i:7:1: internal compiler error: in extract_insn, at recog.c:2123
> Please submit a full bug report,
> with preprocessed source if appropriate.
> See <http://gcc.gnu.org/bugs.html> for instructions.
> make: *** [x.s] Error 1
>
> I will investigate it.

For reference, attached is the complete patch that uses
define_special_predicate. This patch works OK with the current
mainline, with additional patch to i386.h, where


Uros.
Index: i386.c
===================================================================
--- i386.c	(revision 185058)
+++ i386.c	(working copy)
@@ -22952,9 +22952,9 @@ ix86_expand_call (rtx retval, rtx fnaddr, rtx call
 	   : !call_insn_operand (XEXP (fnaddr, 0), Pmode))
     {
       fnaddr = XEXP (fnaddr, 0);
-      if (GET_MODE (fnaddr) != Pmode)
-	fnaddr = convert_to_mode (Pmode, fnaddr, 1);
-      fnaddr = gen_rtx_MEM (QImode, copy_to_mode_reg (Pmode, fnaddr));
+      if (GET_MODE (fnaddr) != word_mode)
+	fnaddr = convert_to_mode (word_mode, fnaddr, 1);
+      fnaddr = gen_rtx_MEM (QImode, copy_to_mode_reg (word_mode, fnaddr));
     }
 
   vec_len = 0;
Index: i386.md
===================================================================
--- i386.md	(revision 185073)
+++ i386.md	(working copy)
@@ -11078,7 +11078,12 @@
    (set_attr "modrm" "0")])
 
 (define_expand "indirect_jump"
-  [(set (pc) (match_operand 0 "indirect_branch_operand" ""))])
+  [(set (pc) (match_operand 0 "indirect_branch_operand" ""))]
+  ""
+{
+  if (TARGET_X32)
+    operands[0] = convert_memory_address (word_mode, operands[0]);
+})
 
 (define_insn "*indirect_jump"
   [(set (pc) (match_operand:P 0 "indirect_branch_operand" "rw"))]
@@ -11123,8 +11128,9 @@
       operands[0] = expand_simple_binop (Pmode, code, op0, op1, NULL_RTX, 0,
 					 OPTAB_DIRECT);
     }
-  else if (TARGET_X32)
-    operands[0] = convert_memory_address (Pmode, operands[0]);
+
+  if (TARGET_X32)
+    operands[0] = convert_memory_address (word_mode, operands[0]);
 })
 
 (define_insn "*tablejump_1"
Index: predicates.md
===================================================================
--- predicates.md	(revision 185073)
+++ predicates.md	(working copy)
@@ -1,5 +1,5 @@
 ;; Predicate definitions for IA-32 and x86-64.
-;; Copyright (C) 2004, 2005, 2006, 2007, 2008, 2009, 2010, 2011
+;; Copyright (C) 2004, 2005, 2006, 2007, 2008, 2009, 2010, 2011, 2012
 ;; Free Software Foundation, Inc.
 ;;
 ;; This file is part of GCC.
@@ -565,22 +565,27 @@
        (match_operand 0 "immediate_operand")))
 
 ;; Test for a valid operand for indirect branch.
-(define_predicate "indirect_branch_operand"
-  (if_then_else (match_test "TARGET_X32")
-    (match_operand 0 "register_operand")
-    (match_operand 0 "nonimmediate_operand")))
+;; Allow register operands in word mode only.
+(define_special_predicate "indirect_branch_operand"
+  (ior (match_test "register_operand
+		     (op, mode == VOIDmode ? mode : word_mode)")
+       (and (not (match_test "TARGET_X32"))
+	    (match_operand 0 "memory_operand"))))
 
 ;; Test for a valid operand for a call instruction.
-(define_predicate "call_insn_operand"
+;; Allow register operands in word mode only.
+(define_special_predicate "call_insn_operand"
   (ior (match_operand 0 "constant_call_address_operand")
-       (match_operand 0 "call_register_no_elim_operand")
+       (match_test "call_register_no_elim_operand
+		     (op, mode == VOIDmode ? mode : word_mode)")
        (and (not (match_test "TARGET_X32"))
 	    (match_operand 0 "memory_operand"))))
 
 ;; Similarly, but for tail calls, in which we cannot allow memory references.
-(define_predicate "sibcall_insn_operand"
+(define_special_predicate "sibcall_insn_operand"
   (ior (match_operand 0 "constant_call_address_operand")
-       (match_operand 0 "register_no_elim_operand")))
+       (match_test "register_no_elim_operand
+		     (op, mode == VOIDmode ? mode : word_mode)")))
 
 ;; Match exactly zero.
 (define_predicate "const0_operand"