[AArch64] Drop poison-generating flags in genSubAdd2SubSub combiner

[antoniofrighetto]

A miscompilation issue has been addressed with improved handling.

Fixes: #88950.

[llvmbot]

@llvm/pr-subscribers-backend-aarch64

Author: Antonio Frighetto (antoniofrighetto)

Changes

A miscompilation issue has been addressed with improved handling.

Fixes: #88950.

---

Full diff: https://github.com/llvm/llvm-project/pull/90028.diff

2 Files Affected:

• (modified) llvm/lib/Target/AArch64/AArch64InstrInfo.cpp (+10-2)
• (modified) llvm/test/CodeGen/AArch64/machine-combiner-subadd2.mir (+27)

diff --git a/llvm/lib/Target/AArch64/AArch64InstrInfo.cpp b/llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
index 7bf06e71a03059..8653f621ba545f 100644
--- a/llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
+++ b/llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
@@ -6924,14 +6924,22 @@ genSubAdd2SubSub(MachineFunction &MF, MachineRegisterInfo &MRI,
     assert((Opcode == AArch64::SUBWrr || Opcode == AArch64::SUBXrr) &&
            "Unexpected instruction opcode.");
 
+  uint32_t RootFlags = Root.getFlags();
+  if (RootFlags & MachineInstr::NoSWrap)
+    Root.clearFlag(MachineInstr::NoSWrap);
+  if (RootFlags & MachineInstr::NoUWrap)
+    Root.clearFlag(MachineInstr::NoUWrap);
+
   MachineInstrBuilder MIB1 =
       BuildMI(MF, MIMetadata(Root), TII->get(Opcode), NewVR)
           .addReg(RegA, getKillRegState(RegAIsKill))
-          .addReg(RegB, getKillRegState(RegBIsKill));
+          .addReg(RegB, getKillRegState(RegBIsKill))
+          .setMIFlags(Root.getFlags());
   MachineInstrBuilder MIB2 =
       BuildMI(MF, MIMetadata(Root), TII->get(Opcode), ResultReg)
           .addReg(NewVR, getKillRegState(true))
-          .addReg(RegC, getKillRegState(RegCIsKill));
+          .addReg(RegC, getKillRegState(RegCIsKill))
+          .setMIFlags(Root.getFlags());
 
   InstrIdxForVirtReg.insert(std::make_pair(NewVR, 0));
   InsInstrs.push_back(MIB1);
diff --git a/llvm/test/CodeGen/AArch64/machine-combiner-subadd2.mir b/llvm/test/CodeGen/AArch64/machine-combiner-subadd2.mir
index d1770bb25fae49..0b09e8a4b5cd38 100644
--- a/llvm/test/CodeGen/AArch64/machine-combiner-subadd2.mir
+++ b/llvm/test/CodeGen/AArch64/machine-combiner-subadd2.mir
@@ -237,3 +237,30 @@ body:              |
     RET_ReallyLR implicit $w0
 
 ...
+---
+# Drop nowrap flags in SUB
+
+# CHECK-LABEL: name: test8
+# CHECK:       %7:gpr64 = SUBXrr %1, %0
+# CHECK-NEXT:  %4:gpr64common = SUBXrr killed %7, killed %2
+
+name:            test8
+registers:
+  - { id: 0, class: gpr64 }
+  - { id: 1, class: gpr64 }
+  - { id: 2, class: gpr64common }
+  - { id: 3, class: gpr64 }
+  - { id: 4, class: gpr64common }
+  - { id: 5, class: gpr64 }
+body:              |
+  bb.0:
+    %1:gpr64 = COPY $x1
+    %0:gpr64 = COPY $x0
+    %2:gpr64common = ORRXri %0:gpr64, 4096
+    %3:gpr64 = ADDXrr killed %2:gpr64common, %0:gpr64
+    %4:gpr64common = nsw SUBSXrr %1:gpr64, killed %3:gpr64, implicit-def dead $nzcv
+    %5:gpr64 = SUBSXri %4:gpr64common, 0, 0, implicit-def $nzcv
+    $x0 = COPY %5:gpr64
+    RET_ReallyLR implicit $x0
+
+...

[davemgreen]

Thanks for putting the patch together.

I think the flags were being added by

llvm-project/llvm/lib/Target/AArch64/AArch64InstrInfo.cpp

Line 7880 in fe47e8f

uint32_t Flags = Root.getFlags();

? If this patch clears the flags on root then they will no longer be set on the new instructions, which is what we want.

It might be worth having the switch at

llvm-project/llvm/lib/Target/AArch64/AArch64InstrInfo.cpp

Line 6969 in fe47e8f

break;

return as opposed to break, so that we don't run the code that is intended for mul+add. Either way this looks correct now, but it would mean that this code doesn't need to modify Root.

[antoniofrighetto]

Thanks for reviewing it!

It might be worth having the switch at

llvm-project/llvm/lib/Target/AArch64/AArch64InstrInfo.cpp

Line 6969 in fe47e8f

break;

return as opposed to break, so that we don't run the code that is intended for mul+add. Either way this looks correct now, but it would mean that this code doesn't need to modify Root.

Right, although clearing the flags makes explicit the semantic of dropping flags in the combiner. I'd personally leave the break, as we would need to anticipate the following in both SUBADD_OP1/SUBADD_OP2 cases otherwise, no strong concerns though.

llvm-project/llvm/lib/Target/AArch64/AArch64InstrInfo.cpp

Line 7876 in fe47e8f

DelInstrs.push_back(&Root);

[v01dXYZ]

@davemgreen Out of curiosity, you seem to prefer not to mutate Root if possible.

Is Root always replaced ? (it doesn't seem so but I prefer to ask).

[davemgreen]

Hi - yeah I was expecting Root to always be replaced, hence it should be correct either way. Not modifying Root feels a little cleaner though. You would not usually expect the old code to be modified as the new nodes are created, and it can be a bit confusing to follow the logic of setting the on the Root, to be used later on (in code that is easy to miss), to override the flags of the instructions that have just been created.

DelInstrs.push_back(&Root);

Yeah I was thinking that would need to be added, either to the switch or into the genSubAdd2SubSub method.

[antoniofrighetto]

Should be on track now!

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[davemgreen]

Thanks. LGTM

[momchil-velikov]

It looks like there's a slight issue with the test. If you have enabled expensive checks, the combiner calls verifyPatternOrder which creates(and discards) a few instructions, but in doing so consumes some virtual register numbers. Then you get unexpected output in the test, for example, I get %9 instead of %7 in these check lines:

# CHECK-LABEL: name: test8
# CHECK:       %7:gpr64 = SUBXrr %1, %0
# CHECK-NEXT:  %4:gpr64common = SUBXrr killed %7, killed %2

[antoniofrighetto]

@momchil-velikov Thank you for reporting, and sorry to get back only now, somehow I missed the notification for this. Fixed in ca25702.