[AArch64InstrInfo] Ignore debug insts in areCFlagsAccessedBetweenInstrs [6/10]

[vsk]

Summary

Fix an issue where the presence of debug info could disable a peephole
optimization due to areCFlagsAccessedBetweenInstrs returning the wrong
result.

In test/CodeGen/AArch64/arm64-csel.ll, the issue was found in the
function @foo5, in which the first compare could successfully be
optimized but not the second.

[danielkiss]

LGTM

[vsk]

Update to use range helper from D78151 (as suggested by @fhahn)

[vsk]

retitle

[vsk]

Undo changes made by Arcanist

[fhahn]

LGTM, thanks

llvm/lib/Target/AArch64/AArch64InstrInfo.cpp

1187

nit: could be reversedInstructionsWithoutDebug(std::prev(To), From).

[vsk]

llvm/lib/Target/AArch64/AArch64InstrInfo.cpp

1187

Thanks, I'll fold this into the change before committing if there aren't any further comments.

[mstorsjo]

This caused a misoptimization for me. With https://martin.st/temp/scene_sad-preproc.c as reproducible source for the issue, I have the following case:

$ clang-good -target aarch64-w64-mingw32 scene_sad-preproc.c -O3 -S -o good
$ clang-bad -target aarch64-w64-mingw32 scene_sad-preproc.c -O3 -S -o bad
$ diff -u good bad
--- good        2020-04-24 11:46:41.372102971 +0300
+++ bad 2020-04-24 11:46:28.440377143 +0300
@@ -156,17 +156,16 @@
        ldrb    w14, [x14, #1]
        add     x12, x12, #2            // =2
        subs    w15, w15, w16
-       sub     w13, w13, w14
+       subs    w13, w13, w14
        cneg    w14, w15, mi
-       cmp     w13, #0                 // =0
        cneg    w13, w13, mi

(With two other unrelated benign differences removed from the diff for clarity.)

In this case, the comparison can't be hoisted into subs past the cneg, as that does use the condition flags.

[vsk]

In D78157#2001436, @mstorsjo wrote:

This caused a misoptimization for me. With https://martin.st/temp/scene_sad-preproc.c as reproducible source for the issue, I have the following case:

$ clang-good -target aarch64-w64-mingw32 scene_sad-preproc.c -O3 -S -o good
$ clang-bad -target aarch64-w64-mingw32 scene_sad-preproc.c -O3 -S -o bad
$ diff -u good bad
--- good        2020-04-24 11:46:41.372102971 +0300
+++ bad 2020-04-24 11:46:28.440377143 +0300
@@ -156,17 +156,16 @@
        ldrb    w14, [x14, #1]
        add     x12, x12, #2            // =2
        subs    w15, w15, w16
-       sub     w13, w13, w14
+       subs    w13, w13, w14
        cneg    w14, w15, mi
-       cmp     w13, #0                 // =0
        cneg    w13, w13, mi

(With two other unrelated benign differences removed from the diff for clarity.)

In this case, the comparison can't be hoisted into subs past the cneg, as that does use the condition flags.

@mstorsjo thanks for reporting this, and sorry about the breakage. The problem is that explicit conversion in MachineInstrBundleIterator for reverse iterators increments the reverse iterator (MachineInstrBundleIterator(++I.getReverse())), and I missed this fact. Actually the comment there explains "resulting iterator will dereference ... to the previous node, which is somewhat unexpected; but converting the two endpoints in a range will give the same range in reverse". This sort of thing seems easy to mess up, and it means 'reversedInstructionsWithoutDebug' will not behave as advertised. I plan to get rid of that helper and check in a regression test.

[vsk]

I've removed reversedInstructionsWithoutDebug and added a regression test in c0fa447e02c4cd8c53c3ab03efa6391175e3d56e.

llvm/lib/CodeGen/PeepholeOptimizer.cpp

bool PeepholeOptimizer::optimizeCmpInstr(MachineInstr &MI) {
   int CmpMask, CmpValue;
   if (!TII->analyzeCompare(MI, SrcReg, SrcReg2, CmpMask, CmpValue) ||
       SrcReg.isPhysical() || SrcReg2.isPhysical())
     return false;
 
   // Attempt to optimize the comparison instruction.
   LLVM_DEBUG(dbgs() << "Attempting to optimize compare: " << MI);
   if (TII->optimizeCompareInstr(MI, SrcReg, SrcReg2, CmpMask, CmpValue, MRI)) {
+    LLVM_DEBUG(dbgs() << "  -> Successfully optimized compare!\n");
     ++NumCmps;
     return true;
   }
 
   return false;
 }
 
 /// Optimize a select instruction.

llvm/lib/Target/AArch64/AArch64InstrInfo.cpp

if (To->getParent() != From->getParent())
     return true;
 
   // From must be above To.
   assert(std::find_if(++To.getReverse(), To->getParent()->rend(),
                       [From](MachineInstr &MI) {
                         return MI.getIterator() == From;
                       }) != To->getParent()->rend());
 
-  // We iterate backward starting \p To until we hit \p From.
-  for (--To; To != From; --To) {
-    const MachineInstr &Instr = *To;
-
+  // We iterate backward starting at \p To until we hit \p From.
+  for (const MachineInstr &Instr :
+       reversedInstructionsWithoutDebug(std::prev(To), From)) {
     if (((AccessToCheck & AK_Write) &&
          Instr.modifiesRegister(AArch64::NZCV, TRI)) ||
         ((AccessToCheck & AK_Read) && Instr.readsRegister(AArch64::NZCV, TRI)))
       return true;
   }
   return false;
 }
 

llvm/test/CodeGen/AArch64/arm64-csel.ll

-; RUN: llc -O3 < %s | FileCheck %s
+; RUN: llc -debugify-and-strip-all-safe -O3 < %s | FileCheck %s
 target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-n32:64"
 target triple = "arm64-unknown-unknown"
 
 ; CHECK-LABEL: foo1
 ; CHECK: cinc w{{[0-9]+}}, w{{[0-9]+}}, ne
 define i32 @foo1(i32 %b, i32 %c) nounwind readnone ssp {
 entry:
   %not.tobool = icmp ne i32 %c, 0