[AArch64ConditionOptimizer] Fix missed optimization due to debug insts [10/10]

[vsk]

Summary

The findSuitableCompare method can fail if debug instructions are
present in the MBB -- fix this by using helpers to skip over debug
insts.

[fhahn]

llvm/lib/Target/AArch64/AArch64ConditionOptimizer.cpp

160–161

Might be worth rewriting in terms of an iterator range, e.g. something like reverseInstructionsWithoutDebug(I == MBB->begin() ? I : std::prev(I), MBB->begin())

[vsk]

llvm/lib/Target/AArch64/AArch64ConditionOptimizer.cpp

160–161

I have a mild preference for the current version as I feel that the ternary adds some complexity. Happy to change this though if anyone has a strong opinion otherwise.

[paquette]

LGTM

[paquette]

llvm/lib/Target/AArch64/AArch64ConditionOptimizer.cpp

160–161

It would be nice to be clear that we're skipping debug instructions in the loop header, I suppose?

I think in general it would be nice to avoid having to explicitly skip debug instructions within the loop itself.

[vsk]

Use a range iterator, per review feedback.

[LukeGeeson]

I cannot build llvm locally using ninja clean && ninja check-all with this patch due to the following error:

AArch64ConditionOptimizer.cpp: In member function 'llvm::MachineInstr* {anonymous}::AArch64ConditionOptimizer::findSuitableCompare(llvm::MachineBasicBlock*) ... no matching function call to 'prev(llvm::MachineInstr&)'

Using the following cmake command in llvm-project/build

cmake -G Ninja -DLLVM_ENABLE_PROJECTS="clang;lld" -DCMAKE_BUILD_TYPE=Debug -DLLVM_ENABLE_ASSERTIONS=On -DLLVM_TARGETS_TO_BUILD="ARM;X86;AArch64" -DLLVM_ENABLE_SOLVER_Z3=Off -DPARALLEL_LINK_JOBS=2 -DLLVM_USE_LINKER=gold ../llvm

It looks as though harbourmaster is failing as well.
Based on commit hash 3a53806 on master

[CarlosAlbertoEnciso]

In D78265#1999052, @LukeGeeson wrote:

I cannot build llvm locally using ninja clean && ninja check-all with this patch due to the following error:

AArch64ConditionOptimizer.cpp: In member function 'llvm::MachineInstr* {anonymous}::AArch64ConditionOptimizer::findSuitableCompare(llvm::MachineBasicBlock*) ... no matching function call to 'prev(llvm::MachineInstr&)'

Using the following cmake command in llvm-project/build

cmake -G Ninja -DLLVM_ENABLE_PROJECTS="clang;lld" -DCMAKE_BUILD_TYPE=Debug -DLLVM_ENABLE_ASSERTIONS=On -DLLVM_TARGETS_TO_BUILD="ARM;X86;AArch64" -DLLVM_ENABLE_SOLVER_Z3=Off -DPARALLEL_LINK_JOBS=2 -DLLVM_USE_LINKER=gold ../llvm

It looks as though harbourmaster is failing as well.
Based on commit hash 3a53806 on master

I have the same problem trying to build llvm locally.

[chill]

Looks like it's a scope error in gcc <= 5.5.
https://gcc.godbolt.org/z/ECqRJJ

The minimum required version to build LLVM GCC 5.1.
https://llvm.org/docs/GettingStarted.html#host-c-toolchain-both-compiler-and-standard-library

I suggest renaming one of the Is.

[CarlosAlbertoEnciso]

In D78265#1999223, @chill wrote:

Looks like it's a scope error in gcc <= 5.5.
https://gcc.godbolt.org/z/ECqRJJ

The minimum required version to build LLVM GCC 5.1.
https://llvm.org/docs/GettingStarted.html#host-c-toolchain-both-compiler-and-standard-library

I have GCC 5.4.

[vsk]

Sorry about the breakage, I've renamed the variable in 210616bd38d589020b45f8cbbf9f9ef1296f2729.

[CarlosAlbertoEnciso]

In D78265#1999445, @vsk wrote:

Sorry about the breakage, I've renamed the variable in 210616bd38d589020b45f8cbbf9f9ef1296f2729.

@vsk Thanks very much for the fix.

llvm/lib/Target/AArch64/AArch64ConditionOptimizer.cpp

MachineInstr *AArch64ConditionOptimizer::findSuitableCompare(
   if (I->getOpcode() != AArch64::Bcc)
     return nullptr;
 
   // Since we may modify cmp of this MBB, make sure NZCV does not live out.
   for (auto SuccBB : MBB->successors())
     if (SuccBB->isLiveIn(AArch64::NZCV))
       return nullptr;
 
   // Now find the instruction controlling the terminator.
-  for (MachineBasicBlock::iterator B = MBB->begin(); I != B;) {
-    --I;
-    assert(!I->isTerminator() && "Spurious terminator");
+  auto B = MBB->begin();
+  for (MachineInstr &I :
+       reversedInstructionsWithoutDebug(I == B ? I : std::prev(I), B)) {
+    assert(!I.isTerminator() && "Spurious terminator");
     // Check if there is any use of NZCV between CMP and Bcc.
-    if (I->readsRegister(AArch64::NZCV))
+    if (I.readsRegister(AArch64::NZCV))
       return nullptr;
-    switch (I->getOpcode()) {
+    switch (I.getOpcode()) {
     // cmp is an alias for subs with a dead destination register.
     case AArch64::SUBSWri:
     case AArch64::SUBSXri:
     // cmn is an alias for adds with a dead destination register.
     case AArch64::ADDSWri:
     case AArch64::ADDSXri: {
-      unsigned ShiftAmt = AArch64_AM::getShiftValue(I->getOperand(3).getImm());
-      if (!I->getOperand(2).isImm()) {
-        LLVM_DEBUG(dbgs() << "Immediate of cmp is symbolic, " << *I << '\n');
+      unsigned ShiftAmt = AArch64_AM::getShiftValue(I.getOperand(3).getImm());
+      if (!I.getOperand(2).isImm()) {
+        LLVM_DEBUG(dbgs() << "Immediate of cmp is symbolic, " << I << '\n');
         return nullptr;
-      } else if (I->getOperand(2).getImm() << ShiftAmt >= 0xfff) {
-        LLVM_DEBUG(dbgs() << "Immediate of cmp may be out of range, " << *I
+      } else if (I.getOperand(2).getImm() << ShiftAmt >= 0xfff) {
+        LLVM_DEBUG(dbgs() << "Immediate of cmp may be out of range, " << I
                           << '\n');
         return nullptr;
-      } else if (!MRI->use_empty(I->getOperand(0).getReg())) {
-        LLVM_DEBUG(dbgs() << "Destination of cmp is not dead, " << *I << '\n');
+      } else if (!MRI->use_nodbg_empty(I.getOperand(0).getReg())) {
+        LLVM_DEBUG(dbgs() << "Destination of cmp is not dead, " << I << '\n');
         return nullptr;
       }
-      return &*I;
+      return &I;
     }
     // Prevent false positive case like:
     // cmp      w19, #0
     // cinc     w0, w19, gt
     // ...
     // fcmp     d8, #0.0
     // b.gt     .LBB0_5
     case AArch64::FCMPDri:

llvm/test/CodeGen/AArch64/combine-comparisons-by-cse.ll

-; RUN: llc < %s -mtriple=aarch64-linux-gnu | FileCheck %s
+; RUN: llc -debugify-and-strip-all-safe < %s -mtriple=aarch64-linux-gnu | FileCheck %s
 
 ; marked as external to prevent possible optimizations
 @a = external global i32
 @b = external global i32
 @c = external global i32
 @d = external global i32
 
 ; (a > 10 && b == c) || (a >= 10 && b == d)