Re: s390: SImode pointers vs LR

[Andreas Krebbel <krebbel at linux dot vnet dot ibm dot com>]

On 06/01/2015 05:06 PM, Jeff Law wrote:
> On 06/01/2015 06:23 AM, Andreas Krebbel wrote:
>> On 05/30/2015 02:57 AM, DJ Delorie wrote:
>>> In config/s390/s390.c we accept addresses that are SImode:
>>>
>>>        if (!REG_P (base)
>>> 	  || (GET_MODE (base) != SImode
>>> 	      && GET_MODE (base) != Pmode))
>>> 	return false;
>>>
>>> However, there doesn't seem to be anything in the s390's opcodes that
>>> masks the top half of address registers in 64-bit mode, the SImode
>>> convention seems to just be a convention for addresses in the first
>>> 4Gb.
>>>
>>> So... what happens if gcc uses a subreg to load the lower half of a
>>> register (via LR), leaving the upper half with random bits in it, then
>>> uses that register as an address?  I could see no code that checked
>>> for this, and I have a fairly large and ungainly test case that says
>>> it breaks :-(
>>>
>>> My local solution was to just disallow "SImode" as an address in
>>> s390_decompose_address, which forces gcc to do an explicit SI->DI
>>> conversion to clear the upper bits, and it seems to work, but I wonder
>>> if it's the ideal solution...
>>
>> We also have address style operands which are used as shift count operands but never as addresses.
>> Accepting SImode there was necessary to prevent reload from messing things up:
>> https://gcc.gnu.org/ml/gcc-patches/2006-03/msg01495.html
>>
>> I don't know if this is still necessary though.
> Looks like a hack to me.  Sadly, no testcase in that BZ, though 
> presumably since it was a bootstrap failure one could checkout a 
> development tree from that era and see the failure.

I've checked r112357 (https://gcc.gnu.org/ml/gcc-patches/2006-03/msg01495.html)
with the SImode check part reverted:

Bootstrap failed with:

/home/andreas/clean/../gcc/gcc/dwarf2out.c: In function ‘constant_size’:
/home/andreas/clean/../gcc/gcc/dwarf2out.c:6572: error: insn does not satisfy its constraints:
(insn 39 38 66 6 /home/andreas/clean/../gcc/gcc/toplev.h:176 (set (reg:SI 2 %r2 [orig:44
prephitmp.4958 ] [44])
        (ashift:SI (reg:SI 2 %r2 [orig:44 prephitmp.4958 ] [44])
            (plus:SI (reg:SI 12 %r12 [49])
                (reg:SI 2 %r2 [orig:44 prephitmp.4958 ] [44])))) 360 {*ashlsi3} (nil)
    (nil))
/home/andreas/clean/../gcc/gcc/dwarf2out.c:6572: internal compiler error: in
reload_cse_simplify_operands, at postreload.c:393

This is the result of reload reloading the constant in:

(insn 58 57 59 6 (set (reg/v:SI 48 [ log ])
        (ashift:SI (reg:SI 66)
            (plus:SI (reg:SI 2 %r2 [+4 ])
                (const_int 1 [0x1])))) 360 {*ashlsi3} (insn_list:REG_DEP_TRUE 53
(insn_list:REG_DEP_TRUE 57 (nil)))
    (expr_list:REG_DEAD (reg:DI 2 %r2)
        (expr_list:REG_DEAD (reg:SI 66)
            (nil))))

Reloads for insn # 58
Reload 0: reload_in (SI) = (const_int 1 [0x1])
        ADDR_REGS, RELOAD_FOR_INPUT (opnum = 2)
        reload_in_reg: (const_int 1 [0x1])
        reload_reg_rtx: (reg:SI 1 %r1 [66])
Reload 1: reload_in (SI) = (reg:SI 1 %r1 [66])
        reload_out (SI) = (reg/v:SI 9 %r9 [orig:48 log ] [48])
        GENERAL_REGS, RELOAD_OTHER (opnum = 0)
        reload_in_reg: (reg:SI 1 %r1 [66])
        reload_out_reg: (reg/v:SI 9 %r9 [orig:48 log ] [48])
        reload_reg_rtx: (reg/v:SI 9 %r9 [orig:48 log ] [48])

Without accepting SImode in s390_decompose_address
find_reloads_address reloads const_int 1 into a reg.  Unfortunately it
assumes that this makes a valid address because double_reg_address_ok
is true on our target. But of course the shift instruction cannot deal
with an index register.

However, the problem to me appears to be that s390_decompose_address
is involved for shift count operands at all.  We have a separate
function for this (s390_decompose_shift_count) which is invoked by the
relevant predicate and constraint. s390_decompose_address is only
called because the Y constraint letter is marked as
EXTRA_ADDRESS_CONSTRAINT.

So the obvious solution appears to be to remove 'Y' from the
EXTRA_ADDRESS_CONSTRAINTS.  But this is not going to work that easily
since we rely on reload picking a valid base register. As with
addresses we have to avoid r0 here since it stands for an immediate 0
instead of the content of r0.  The legitimate_address_p hook together
with find_reloads_address takes care of this for us.

Since the operand here is not simply a register we cannot easily make
reload to pick one from the ADDR_REGS register class via constraint.

The only way to address this while still exploiting the various
formats of the shift count operand is probably to split the patterns
up into three different variants.  This then would look like the
attached patch where I did it for only one of the instructions:

diff --git a/gcc/config/s390/s390.c b/gcc/config/s390/s390.c
index 0d03fa6..c5e70ee 100644
--- a/gcc/config/s390/s390.c
+++ b/gcc/config/s390/s390.c
@@ -1617,9 +1617,7 @@ s390_decompose_address (rtx addr, struct s390_address *out)
 	    return false;
 	  }

-      if (!REG_P (base)
-	  || (GET_MODE (base) != SImode
-	      && GET_MODE (base) != Pmode))
+      if (!REG_P (base) || GET_MODE (base) != Pmode)
 	return false;

       if (REGNO (base) == STACK_POINTER_REGNUM
@@ -1665,9 +1663,7 @@ s390_decompose_address (rtx addr, struct s390_address *out)
 	    return false;
 	  }

-      if (!REG_P (indx)
-	  || (GET_MODE (indx) != SImode
-	      && GET_MODE (indx) != Pmode))
+      if (!REG_P (indx) || GET_MODE (indx) != Pmode)
 	return false;

       if (REGNO (indx) == STACK_POINTER_REGNUM
diff --git a/gcc/config/s390/s390.h b/gcc/config/s390/s390.h
index 5feb9d5..bb49a5a 100644
--- a/gcc/config/s390/s390.h
+++ b/gcc/config/s390/s390.h
@@ -528,7 +528,7 @@ extern const enum reg_class regclass_map[FIRST_PSEUDO_REGISTER];
 #define EXTRA_MEMORY_CONSTRAINT(C, STR)					\
   ((C) == 'Q' || (C) == 'R' || (C) == 'S' || (C) == 'T' || (C) == 'A')
 #define EXTRA_ADDRESS_CONSTRAINT(C, STR)				\
-  ((C) == 'U' || (C) == 'W' || (C) == 'Y')
+  ((C) == 'U' || (C) == 'W')

 #define CONSTRAINT_LEN(C, STR)                                  	\
   ((C) == 'N' ? 5 : 	                                                \
diff --git a/gcc/config/s390/s390.md b/gcc/config/s390/s390.md
index c03f880..671625f 100644
--- a/gcc/config/s390/s390.md
+++ b/gcc/config/s390/s390.md
@@ -6313,12 +6313,32 @@
   [(set_attr "op_type"  "RS")
    (set_attr "atype"    "reg")])

-(define_insn "*<shift><mode>3"
+(define_insn "*<shift><mode>3_reg"
   [(set (match_operand:GPR 0 "register_operand" "=d")
         (SHIFT:GPR (match_operand:GPR 1 "register_operand" "<d0>")
-                   (match_operand:SI 2 "shift_count_or_setmem_operand" "Y")))]
+                   (match_operand:SI 2 "register_operand" "a")))]
   ""
-  "s<lr>l<g>\t%0,<1>%Y2"
+  "s<lr>l<g>\t%0,<1>%2"
+  [(set_attr "op_type"  "RS<E>")
+   (set_attr "atype"    "reg")])
+
+(define_insn "*<shift><mode>3_imm"
+  [(set (match_operand:GPR 0 "register_operand" "=d")
+        (SHIFT:GPR (match_operand:GPR 1 "register_operand" "<d0>")
+                   (match_operand     2 "immediate_operand" "J")))]
+  ""
+  "s<lr>l<g>\t%0,<1>%2"
+  [(set_attr "op_type"  "RS<E>")
+   (set_attr "atype"    "reg")])
+
+(define_insn "*<shift><mode>3_immreg"
+  [(set (match_operand:GPR 0 "register_operand" "=d")
+        (SHIFT:GPR (match_operand:GPR 1 "register_operand" "<d0>")
+		   (plus:SI
+		    (match_operand:SI 2 "register_operand"    "a")
+		    (match_operand    3 "immediate_operand"   "J"))))]
+  ""
+  "s<lr>l<g>\t%0,<1>%2(%3)"
   [(set_attr "op_type"  "RS<E>")
    (set_attr "atype"    "reg")])


With this we still get:

shift:
        sll     %r2,%r3(3)
        llgfr   %r2,%r2
        br      %r14

for:

unsigned int
shift (unsigned int s, unsigned  int a)
{
  return s << (a + 3);
}

I'm still hoping for a better idea though ...

Bye,

-Andreas-