[PATCH] D29896: [BypassSlowDivision] Refactor fast division insertion logic (NFC)

[Justin Lebar via Phabricator via llvm-commits]

jlebar added inline comments.


================
Comment at: lib/Transforms/Utils/BypassSlowDivision.cpp:139
+};
+}
+
----------------
Can we add a "// anonymous namespace" comment?


================
Comment at: lib/Transforms/Utils/BypassSlowDivision.cpp:346
+  FastDivInserter FDI(BypassWidths);
+  FastDivInsertionTask Task;
 
----------------
I'm totally behind the notion of creating classes that encapsulate our temporary state.  But I'm not wild about the way we use FastDivInsertionTask as basically a bucket of *mutable* state.  We have had really nasty bugs in LLVM with similar designs where we forget to reset one piece of the mutable state.  (I think Chandler was saying that some pass effectively had a global cost cap instead of a per-function cost cap, because they just forgot to set the cost to 0 at the end of the function.)

I went through a few iterations, and the one that seems best to me right now is to get rid of FastDivInserter, which is already very simple, and make FastDivInsertionTask a one-shot class.

  DivCacheTy Cache;
  while (Next != nullptr) {
    Instruction *I = Next;
    Next = Next->getNextNode();

    if (Value *Replacement = FastDivInsertionTask(I, Cache, BypassWidths).getReplacement()) {
      I->RAUW(Replacement);
      MadeChange |= true;
    }
  }

WDYT?


https://reviews.llvm.org/D29896