[SystemZ] Bad codegen due to EXRL clobbering CC

[uweigand]

The ClickHouse team reported a potential compiler bug. I managed to extract the attached test case
test.cpp.txt
from this file:
https://github.com/ClickHouse/ClickHouse/blob/master/src/Compression/CompressionCodecT64.cpp

To reproduce the problem, build the file using clang++ -std=c++20 -Wall -g -O3 -o test test.cpp -march=z13 and run it. The output should be 0 1 - if it is anything else, we see the bug.

Debugging this showed the problem is caused by the code used to the implement a variable-length memset to zero. The final part after the loop is done using an EXRL targeting a XC instruction. Since XC clobbers the condition code, so does this EXRL. However, the instruction was not marked as doing so, causing the instruction scheduler to move the EXRL between a CC def and its use.

Note there is code in SystemZTargetLowering::emitMemMemWrapper to mark an EXRL as clobbering CC; but this done only if the target is CLC, not if the target is NC, OC, or XC.

[uweigand]

@JonPsson you make the last set of changes to emitMemMemWrapper - could you have a look? I've the following test patch:

diff --git a/llvm/lib/Target/SystemZ/SystemZISelLowering.cpp b/llvm/lib/Target/SystemZ/SystemZISelLowering.cpp
index 256c0fa036b4..909e9ae92220 100644
--- a/llvm/lib/Target/SystemZ/SystemZISelLowering.cpp
+++ b/llvm/lib/Target/SystemZ/SystemZISelLowering.cpp
@@ -8502,9 +8502,10 @@ SystemZTargetLowering::emitMemMemWrapper(MachineInstr &MI,
           .addReg(RemSrcReg).addImm(SrcDisp);
       MBB->addSuccessor(AllDoneMBB);
       MBB = AllDoneMBB;
-      if (EndMBB) {
+      if (Opcode != SystemZ::MVC) {
         EXRL_MIB.addReg(SystemZ::CC, RegState::ImplicitDefine);
-        MBB->addLiveIn(SystemZ::CC);
+        if (EndMBB)
+          MBB->addLiveIn(SystemZ::CC);
       }
     }
   }

which does appear to fix the issue. The only change to generated code is this:

--- test-bad.s  2023-05-05 17:40:01.129102093 +0200
+++ test-good.s 2023-05-05 17:41:11.579137164 +0200
@@ -32651,8 +32651,8 @@
        #DEBUG_VALUE: decompressData<unsigned int, false>:src <- $r2d
        #DEBUG_VALUE: decompressData<unsigned int, false>:buf <- [DW_OP_plus_uconst 2416, DW_OP_deref] $r15d
        lay     %r1, -1(%r1)
-       cghi    %r1, -1
        exrl    %r3, .Ltmp627
+       cghi    %r1, -1
        jge     .LBB0_65
 .Ltmp763:
 .LBB0_191:                              # %if.then22.i46.i
@@ -34963,8 +34963,8 @@
        #DEBUG_VALUE: decompressData<unsigned char, false>:dst <- [DW_OP_plus_uconst 2936] [$r15d+0]
        #DEBUG_VALUE: decompressData<unsigned char>:dst <- [DW_OP_plus_uconst 2936] [$r15d+0]
        lay     %r1, -1(%r1)
-       cghi    %r1, -1
        exrl    %r3, .Ltmp627
+       cghi    %r1, -1
        jge     .LBB0_79
 .Ltmp839:
 .LBB0_218:                              # %if.then25.i47.i

which looks correct to me.

Once fixed, this should ideally also be backported to the LLVM 16 and 15 branches.

[JonPsson]

Huh - that seems like a bug to me as well. The EXRL_Pseudo has the side-effects flag which means it is not safe to move and perhaps made for a false sense of safety here. From what I understand, the branch probability block placement pass duplicates the successor block and places the EXRL_Pseudo first in it. The post-RA scheduler then later reschedules with TMLL as that instruction does not touch memory.

I will post your patch once I have a good test case as well for it.

[JonPsson]

https://reviews.llvm.org/D150251

[uweigand]

Can you do a backport to LLVM 16 as well, please?

(I guess LLVM 15 doesn't make sense at this point since there won't be any further releases on that branch ...)

[JonPsson]

/cherry-pick 655f0fc

[llvmbot]

/branch llvm/llvm-project-release-prs/issue62572

[llvmbot]

/pull-request llvm/llvm-project-release-prs#442

[JonPsson]

Can you do a backport to LLVM 16 as well, please?

(I guess LLVM 15 doesn't make sense at this point since there won't be any further releases on that branch ...)

Backported with 83c2387.

[JonPsson1]

It seems this was backported only to LLVM 16, but should also go into LLVM 15...

[EugeneZelenko]

@JonPsson: Only most recent release is maintained (16 at this particular case).