llvm dev - Nov 2017 - TwoAddressInstructionPass bug?

Hi,

we are in the midst of an interesting work that begun with setting 
'guessInstructionProperties = 0' in the SystemZ backend. We have found 
this to be useful, and discovered many instructions where the 
hasSideEffects flag was incorrectly set while it actually shouldn't.

The attached patch and test case triggers an assert in TwoAddress.  
(bin/llc ./tc_TwoAddr_crash.ll -mtriple=s390x-linux-gnu -mcpu=z13)

The only change in the patch is to remove the side effects flag from one 
instruction:

-  def RISBMux : RotateSelectRIEfPseudo<GRX32, GRX32>;
+  let hasSideEffects = 0 in
+    def RISBMux : RotateSelectRIEfPseudo<GRX32, GRX32>;

The input to TwoAddress is:

BB#0: derived from LLVM BB %0
     Live Ins: %r2l
         %vreg0<def> = COPY %r2l<kill>; GR32Bit:%vreg0
         %vreg9<def,tied1> = NIFMux %vreg0<tied0>, 14, 
%cc<imp-def,dead>; GRX32Bit:%vreg9 GR32Bit:%vreg0
         %vreg4<def,tied1> = NIFMux %vreg0<tied0>, 254, 
%cc<imp-def,dead>; GRX32Bit:%vreg4 GR32Bit:%vreg0
         %vreg2<def> = COPY %vreg0<kill>; GR32Bit:%vreg2,%vreg0
         %vreg3<def> = COPY %vreg9<kill>; GR32Bit:%vreg3
GRX32Bit:%vreg9

         ...

The NIFMux instructions will be converted by the SystemZ backend into 
RISBMux instructions. The tied source operand of the RISBMux is 0 
(no-register / undef). This seems necessary as this is after 'Process 
implicit defs' pass.

The big change with the patch is that TwoAddress is now able to 
reschedule the RISBMux close to the killing instruction:

1: MBB->dump() = BB#0: derived from LLVM BB %0
     Live Ins: %r2l
         %vreg0<def> = COPY %r2l<kill>; GR32Bit:%vreg0
         %vreg4<def,tied1> = NIFMux %vreg0<tied0>, 254, 
%cc<imp-def,dead>; GRX32Bit:%vreg4 GR32Bit:%vreg0
         %vreg2<def> = COPY %vreg0; GR32Bit:%vreg2,%vreg0
         %vreg9<def,tied1> = RISBMux %noreg<tied0>,
%vreg0<kill>, 28,
158, 0; GRX32Bit:%vreg9 GR32Bit:%vreg0
         %vreg3<def> = COPY %vreg9<kill>; GR32Bit:%vreg3
GRX32Bit:%vreg9

         ...

This however means that TwoAddress will encounter this instruction once 
more as it iterates through MBB. That's when the assert triggers:

assert(SrcReg && SrcMO.isUse() && "two address instruction
invalid");

It seems to me that since the %noreg makes sense (per above), and 
TwoAddress wants to schedule this instruction down the block, this 
assert is wrong here.

Should TwoAddress have a set of rescheduled instructions and not revisit 
them while iterating over MBB? Or is target allowed to actually build 
new 2-addr instructions here (seems odd)?

One alternative might be to first make a set of original instructions 
and only process them.

/Jonas



-------------- next part --------------
A non-text attachment was scrubbed...
Name: RISBMux_noSEFlag.patch
Type: text/x-patch
Size: 728 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20171130/6c2aea24/attachment.bin>
-------------- next part --------------
; ModuleID =
'/home/ijonpan/llvm/llvm-dev/test/CodeGen/SystemZ/asm-18.ll'
source_filename =
"/home/ijonpan/llvm/llvm-dev/test/CodeGen/SystemZ/asm-18.ll"

define i32 @f23(i32 %old) {
  %and1 = and i32 %old, 14
  %and2 = and i32 %old, 254
  %res1 = call i32 asm "stepa $1, $2, $3", "=h,r,r,0"(i32
%old, i32 %and1, i32 %and2)
  %and3 = and i32 %res1, 127
  %and4 = and i32 %res1, 128
  %res2 = call i32 asm "stepb $1, $2, $3", "=r,h,h,0"(i32
%res1, i32 %and3, i32 %and4)
  ret i32 %res2
}

Hi Jonas,

Thanks for bringing that up.
> On Nov 30, 2017, at 4:04 AM, Jonas Paulsson via llvm-dev <llvm-dev at
lists.llvm.org> wrote:
> 
> Hi,
> 
> we are in the midst of an interesting work that begun with setting
'guessInstructionProperties = 0' in the SystemZ backend. We have found
this to be useful, and discovered many instructions where the hasSideEffects
flag was incorrectly set while it actually shouldn't.
> 
> The attached patch and test case triggers an assert in TwoAddress. 
(bin/llc ./tc_TwoAddr_crash.ll -mtriple=s390x-linux-gnu -mcpu=z13)
> 
> The only change in the patch is to remove the side effects flag from one
instruction:
> 
> -  def RISBMux : RotateSelectRIEfPseudo<GRX32, GRX32>;
> +  let hasSideEffects = 0 in
> +    def RISBMux : RotateSelectRIEfPseudo<GRX32, GRX32>;
> 
> The input to TwoAddress is:
> 
> BB#0: derived from LLVM BB %0
>     Live Ins: %r2l
>         %vreg0<def> = COPY %r2l<kill>; GR32Bit:%vreg0
>         %vreg9<def,tied1> = NIFMux %vreg0<tied0>, 14,
%cc<imp-def,dead>; GRX32Bit:%vreg9 GR32Bit:%vreg0
>         %vreg4<def,tied1> = NIFMux %vreg0<tied0>, 254,
%cc<imp-def,dead>; GRX32Bit:%vreg4 GR32Bit:%vreg0
>         %vreg2<def> = COPY %vreg0<kill>; GR32Bit:%vreg2,%vreg0
>         %vreg3<def> = COPY %vreg9<kill>; GR32Bit:%vreg3
GRX32Bit:%vreg9
> 
>         ...
> 
> The NIFMux instructions will be converted by the SystemZ backend into
RISBMux instructions. The tied source operand of the RISBMux is 0 (no-register /
undef). This seems necessary as this is after 'Process implicit defs'
pass.
> 
> The big change with the patch is that TwoAddress is now able to reschedule
the RISBMux close to the killing instruction:
> 
> 1: MBB->dump() = BB#0: derived from LLVM BB %0
>     Live Ins: %r2l
>         %vreg0<def> = COPY %r2l<kill>; GR32Bit:%vreg0
>         %vreg4<def,tied1> = NIFMux %vreg0<tied0>, 254,
%cc<imp-def,dead>; GRX32Bit:%vreg4 GR32Bit:%vreg0
>         %vreg2<def> = COPY %vreg0; GR32Bit:%vreg2,%vreg0
>         %vreg9<def,tied1> = RISBMux %noreg<tied0>,
%vreg0<kill>, 28, 158, 0; GRX32Bit:%vreg9 GR32Bit:%vreg0
>         %vreg3<def> = COPY %vreg9<kill>; GR32Bit:%vreg3
GRX32Bit:%vreg9
> 
>         ...
> 
> This however means that TwoAddress will encounter this instruction once
more as it iterates through MBB. That's when the assert triggers:
> 
> assert(SrcReg && SrcMO.isUse() && "two address
instruction invalid");
> 
> It seems to me that since the %noreg makes sense (per above), and
TwoAddress wants to schedule this instruction down the block, this assert is
wrong here.
I agree.
> 
> Should TwoAddress have a set of rescheduled instructions and not revisit
them while iterating over MBB? Or is target allowed to actually build new 2-addr
instructions here (seems odd)?
Having a set of rescheduled instructions and skipping them sounds reasonable to
me.

Cheers,
-Quentin
> 
> One alternative might be to first make a set of original instructions and
only process them.
> 
> /Jonas
> 
> 
> 
>
<RISBMux_noSEFlag.patch><tc_TwoAddr_crash.ll>_______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

Hi Quentin,

> Having a set of rescheduled instructions and skipping them sounds
reasonable to me.
>
> Cheers,
> -Quentin
All right, please see https://reviews.llvm.org/D40711, which seems to do 
the trick.

/Jonas