[PATCH] D30416: [BitfieldShrinking] Shrink Bitfields load/store when the bitfields are legal to access independently

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added a comment.

I'm still working on this, but since Wei mentioned he is looking at fixing the `CandidatesToErase` stuff, I wanted to go ahead and send these comments -- there is a significant one w.r.t. to the deletion stuff as well.



================
Comment at: lib/CodeGen/MemAccessShrinking.cpp:84-85
+
+static cl::opt<bool> EnableLoadShrink("enable-load-shrink", cl::init(true));
+static cl::opt<bool> EnableStoreShrink("enable-store-shrink", cl::init(true));
+
----------------
Are both of these really useful for debugging? We already have a flag that controls whether the pass is enabled or not.


================
Comment at: lib/CodeGen/MemAccessShrinking.cpp:88-89
+namespace {
+/// Describe the value range used for mem access.
+struct ModRange {
+  unsigned Shift;
----------------
"mod" range seems like an odd name?


================
Comment at: lib/CodeGen/MemAccessShrinking.cpp:92
+  unsigned Width;
+  bool ShrinkWithMaskedVal;
+};
----------------
Maybe a comment on what this is used for?


================
Comment at: lib/CodeGen/MemAccessShrinking.cpp:99-100
+  static char ID; // Pass identification, replacement for typeid
+  MemAccessShrinkingPass(const TargetMachine *TM = nullptr) : FunctionPass(ID) {
+    this->TM = TM;
+    initializeMemAccessShrinkingPassPass(*PassRegistry::getPassRegistry());
----------------
We generally use initializer sequences:

  MemAccessShrinkingPass(const TargetMachine *TM = nullptr)
      : FunctionPass(ID), TM(TM) {

Also, sense this is an internal type I'd skip the default argument for `TM` as it seems to not really give you much.


================
Comment at: lib/CodeGen/MemAccessShrinking.cpp:131
+
+  // MultiUsesSeen shows a multiuse node is found on the du-chain.
+  bool MultiUsesSeen;
----------------
I would try to expand unusual acronyms: 'du-chain' -> 'Def-Use chain'.


================
Comment at: lib/CodeGen/MemAccessShrinking.cpp:983-996
+/// Check insts in CandidatesToErase. If they are dead, remove the dead
+/// instructions recursively and clean up Memory SSA.
+bool MemAccessShrinkingPass::removeDeadInsts() {
+  SmallVector<Instruction *, 16> DeadInsts;
+  bool Changed = false;
+
+  for (Instruction *I : CandidatesToErase) {
----------------
Rather than reproduce the body of `RecursivelyDeleteTriviallyDeadInstructions` here, how about actually refactoring that routine to have an overload taht accepts a SmallVectorImpl<Instruction *> list of dead instructions, then you can hand your list to this routine rather than writing your own.

Specifically, I think (as Eli is alluding to below) you should only put things into your `CandidatesToErase` vector (which I would rename `DeadInsts` or something) when they satisfy `isInstructionTriviallyDead`. Even if deleting one of the instructions is necessary to make one of the candidates dead, we'll still visit it because these routines *recursively* delete dead instructions already.


================
Comment at: lib/CodeGen/MemAccessShrinking.cpp:1051-1053
+  bool MadeChange = true;
+  bool EverMadeChange = false;
+  while (MadeChange) {
----------------
This pattern seems confusing. How about using a lambda (or even an actual separate function) to model a single pass over the function, so that it can just return a single `Changed` variable?


================
Comment at: lib/CodeGen/MemAccessShrinking.cpp:1056-1058
+      auto CurInstIterator = BB->rbegin();
+      while (CurInstIterator != BB->rend())
+        MadeChange |= tryShrinkOnInst(*CurInstIterator++);
----------------
I would still use a for loop here, and importantly capture rend early:

  for (auto InstI = BB->rbegin(), InstE = BB-rend(); InstI != InstE;)
    ...(*InstI++);



================
Comment at: lib/CodeGen/MemAccessShrinking.cpp:1061
+    MadeChange |= removeDeadInsts();
+    EverMadeChange |= MadeChange;
+  }
----------------
Why not just one `Changed` variable?


Repository:
  rL LLVM

https://reviews.llvm.org/D30416