[llvm-dev] TwoAddressInstructionPass bug?

[Jonas Paulsson via llvm-dev]

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
}