[PATCH] D70091: [BranchFolding] Fix PR43964 about branch folder not being debug invariant

[Jeremy Morse via Phabricator via llvm-commits]

jmorse added a comment.

This looks good to me and great refactor. To make sure I understand what's going on: by keeping a temporary iterator in ComputeCommonTailLength, and only "committing" the advance to I1/I2 when we're sure we've found some additional identical tail, this avoids all the concerns about I1/I2 ending up pointing at pseudo instructions?



================
Comment at: llvm/lib/CodeGen/BranchFolding.cpp:320
   unsigned TailLen = 0;
-  while (I1 != MBB1->begin() && I2 != MBB2->begin()) {
-    --I1; --I2;
----------------
I endorse the vast volume of red in this patch :)


================
Comment at: llvm/lib/CodeGen/BranchFolding.cpp:323-325
+/// Iterate backwards from the given iterator \p I, towards the beginning of the
+/// block. If a MI satisfying 'isDebugInstr' is found, return an iterator
+/// pointing to that MI. If no such MI is found, return the end iterator.
----------------
It looks like there's a `skipDebugInstructionsBackward` helper in MachineBasicBlock.h already (I'd never heard of it before I went digging). Is it worth reusing that -- it does have a different return value once it hits 'begin'.


================
Comment at: llvm/lib/CodeGen/BranchFolding.cpp:324
+/// Iterate backwards from the given iterator \p I, towards the beginning of the
+/// block. If a MI satisfying 'isDebugInstr' is found, return an iterator
+/// pointing to that MI. If no such MI is found, return the end iterator.
----------------
*not* satisfying?


================
Comment at: llvm/lib/CodeGen/BranchFolding.cpp:352
   unsigned TailLen = 0;
-  while (I1 != MBB1->begin() && I2 != MBB2->begin()) {
-    --I1; --I2;
-    // Skip debugging pseudos; necessary to avoid changing the code.
-    while (!countsAsInstruction(*I1)) {
-      if (I1==MBB1->begin()) {
-        while (!countsAsInstruction(*I2)) {
-          if (I2==MBB2->begin()) {
-            // I1==DBG at begin; I2==DBG at begin
-            goto SkipTopCFIAndReturn;
-          }
-          --I2;
-        }
-        ++I2;
-        // I1==DBG at begin; I2==non-DBG, or first of DBGs not at begin
-        goto SkipTopCFIAndReturn;
-      }
-      --I1;
-    }
-    // I1==first (untested) non-DBG preceding known match
-    while (!countsAsInstruction(*I2)) {
-      if (I2==MBB2->begin()) {
-        ++I1;
-        // I1==non-DBG, or first of DBGs not at begin; I2==DBG at begin
-        goto SkipTopCFIAndReturn;
-      }
-      --I2;
-    }
-    // I1, I2==first (untested) non-DBGs preceding known match
-    if (!I1->isIdenticalTo(*I2) ||
+  for (;;) {
+    MBBI1 = skipBackwardPastNonInstructions(MBBI1, MBB1);
----------------
while (true)?


================
Comment at: llvm/lib/CodeGen/BranchFolding.cpp:363-364
         // directives.
-        I1->isInlineAsm()) {
-      ++I1; ++I2;
+        // FIXME: If inline asm check really is needed, then why don't we also
+        // check MBBI2?
+        MBBI1->isInlineAsm()) {
----------------
Presumably if we pass isIdenticalTo, MBBI2 must presumably point at a duplicate INLINE_ASM instruction?


================
Comment at: llvm/test/CodeGen/X86/branchfolding-debug-invariant.mir:1
+# RUN: llc -mtriple=x86_64-- -run-pass branch-folder -O3 -o - %s | FileCheck %s
+
----------------
I'm enjoying the use of DBG_VALUEs with no operands, avoiding any metdata or IR in these tests!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70091/new/

https://reviews.llvm.org/D70091