[llvm] c0fa447 - AArch64: Remove reversedInstructionsWithoutDebug helper

[Vedant Kumar via llvm-commits]

Author: Vedant Kumar
Date: 2020-04-24T11:28:17-07:00
New Revision: c0fa447e02c4cd8c53c3ab03efa6391175e3d56e

URL: https://github.com/llvm/llvm-project/commit/c0fa447e02c4cd8c53c3ab03efa6391175e3d56e
DIFF: https://github.com/llvm/llvm-project/commit/c0fa447e02c4cd8c53c3ab03efa6391175e3d56e.diff

LOG: 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.

Added: 
    llvm/test/CodeGen/AArch64/peephole-opt-check-cflags.mir

Modified: 
    llvm/include/llvm/CodeGen/MachineBasicBlock.h
    llvm/lib/Target/AArch64/AArch64ConditionOptimizer.cpp
    llvm/lib/Target/AArch64/AArch64InstrInfo.cpp

Removed: 
    


################################################################################
diff  --git a/llvm/include/llvm/CodeGen/MachineBasicBlock.h b/llvm/include/llvm/CodeGen/MachineBasicBlock.h
index d6c7d508b535..2788ef5c0103 100644
--- 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 eda74b1b2e3b..f8a3bd6ecbcf 100644
--- 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 aabe4b106de6..678fe0502fb4 100644
--- 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
index 000000000000..d8d9b64b0116
--- /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
+
+...