AArch64: Remove reversedInstructionsWithoutDebug helper

When using reversedInstructionsWithoutDebug to construct a range from a
pair of MachineInstrBundleIterators, the range unexpectedly leaves out an
element. This results in mis-optimization as @mstorsjo points out in
https://reviews.llvm.org/D78157.

The problem is that when we convert a MachineInstrBundleIterator to a
reverse iterator, the result gets incremented:

  MachineInstrBundleIterator(++I.getReverse())

The comment there explains that the "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
makes it hard to understand what reversedInstructionsWithoutDebug will
do: I've removed the helper to prevent similar mistakes in the future.

diff --git a/llvm/include/llvm/CodeGen/MachineBasicBlock.h b/llvm/include/llvm/CodeGen/MachineBasicBlock.h
index d6c7d50..2788ef5 100644 (file)
--- a/llvm/include/llvm/CodeGen/MachineBasicBlock.h
+++ b/llvm/include/llvm/CodeGen/MachineBasicBlock.h
@@ -1082,14 +1082,6 @@ inline auto instructionsWithoutDebug(IterT It, IterT End) {
   });
 }
 
-/// Construct a range iterator which begins at \p It and moves backwards until
-/// \p Begin is reached, skipping any debug instructions.
-template <typename IterT>
-inline auto reversedInstructionsWithoutDebug(IterT It, IterT Begin) {
-  return instructionsWithoutDebug(make_reverse_iterator(It),
-                                  make_reverse_iterator(Begin));
-}
-
 } // end namespace llvm
 
 #endif // LLVM_CODEGEN_MACHINEBASICBLOCK_H

diff --git a/llvm/lib/Target/AArch64/AArch64ConditionOptimizer.cpp b/llvm/lib/Target/AArch64/AArch64ConditionOptimizer.cpp
index eda74b1..f8a3bd6 100644 (file)
--- a/llvm/lib/Target/AArch64/AArch64ConditionOptimizer.cpp
+++ b/llvm/lib/Target/AArch64/AArch64ConditionOptimizer.cpp
@@ -158,9 +158,9 @@ MachineInstr *AArch64ConditionOptimizer::findSuitableCompare(
       return nullptr;
 
   // Now find the instruction controlling the terminator.
-  auto B = MBB->begin();
-  for (MachineInstr &I : reversedInstructionsWithoutDebug(
-           Term == B ? Term : std::prev(Term), B)) {
+  for (MachineBasicBlock::iterator B = MBB->begin(), It = Term; It != B;) {
+    It = prev_nodbg(It, B);
+    MachineInstr &I = *It;
     assert(!I.isTerminator() && "Spurious terminator");
     // Check if there is any use of NZCV between CMP and Bcc.
     if (I.readsRegister(AArch64::NZCV))

diff --git a/llvm/lib/Target/AArch64/AArch64InstrInfo.cpp b/llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
index aabe4b1..678fe05 100644 (file)
--- a/llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
+++ b/llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
@@ -1185,7 +1185,7 @@ static bool areCFlagsAccessedBetweenInstrs(
 
   // We iterate backward starting at \p To until we hit \p From.
   for (const MachineInstr &Instr :
-       reversedInstructionsWithoutDebug(std::prev(To), From)) {
+       instructionsWithoutDebug(++To.getReverse(), From.getReverse())) {
     if (((AccessToCheck & AK_Write) &&
          Instr.modifiesRegister(AArch64::NZCV, TRI)) ||
         ((AccessToCheck & AK_Read) && Instr.readsRegister(AArch64::NZCV, TRI)))

diff --git a/llvm/test/CodeGen/AArch64/peephole-opt-check-cflags.mir b/llvm/test/CodeGen/AArch64/peephole-opt-check-cflags.mir
new file mode 100644 (file)
index 0000000..d8d9b64
--- /dev/null
+++ b/llvm/test/CodeGen/AArch64/peephole-opt-check-cflags.mir
@@ -0,0 +1,54 @@
+# RUN: llc %s -o - -run-pass=peephole-opt | FileCheck %s
+
+# Check that we don't optimize out a subs due to areCFlagsAccessedBetweenInstrs
+# returning the wrong result; it should check the cneg before the subs which does
+# modify cflags.
+
+# CHECK-LABEL: f
+# CHECK:      SUBSWrr
+# CHECK-NEXT: SUBWrr
+# CHECK-NEXT: CSNEGWr
+# CHECK-NEXT: SUBSWri
+# CHECK-NEXT: CSNEGWr
+# CHECK-NEXT: Bcc
+
+--- |
+  target datalayout = "e-m:w-p:64:64-i32:32-i64:64-i128:128-n32:64-S128"
+  target triple = "aarch64-w64-windows-gnu"
+
+  define dso_local void @f() {
+    ret void
+  }
+
+...
+---
+name:            f
+registers:
+  - { id: 43, class: gpr32, preferred-register: '' }
+  - { id: 44, class: gpr32, preferred-register: '' }
+  - { id: 46, class: gpr32, preferred-register: '' }
+  - { id: 47, class: gpr32, preferred-register: '' }
+  - { id: 48, class: gpr32common, preferred-register: '' }
+  - { id: 49, class: gpr32common, preferred-register: '' }
+  - { id: 50, class: gpr32, preferred-register: '' }
+  - { id: 51, class: gpr32, preferred-register: '' }
+  - { id: 52, class: gpr32, preferred-register: '' }
+  - { id: 53, class: gpr32, preferred-register: '' }
+body:             |
+  bb.0:
+    successors: %bb.0
+
+    %43 = MOVi32imm 1
+    %44 = MOVi32imm 1
+    %46 = MOVi32imm 1
+    %47 = MOVi32imm 1
+    %48 = nsw SUBSWrr killed %43, killed %46, implicit-def dead $nzcv
+    %49 = nsw SUBSWrr killed %44, killed %47, implicit-def dead $nzcv
+    %50 = SUBSWri %48, 0, 0, implicit-def $nzcv
+    %51 = CSNEGWr %48, %48, 5, implicit $nzcv
+    %52 = SUBSWri %49, 0, 0, implicit-def $nzcv
+    %53 = CSNEGWr %49, %49, 5, implicit $nzcv
+    Bcc 1, %bb.0, implicit $nzcv
+    RET_ReallyLR
+
+...