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

[Wei Mi via Phabricator via llvm-commits]

wmi added a comment.

Thanks for bearing with my poor English. I will fix the terminologies and comments according to your suggestions.



================
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));
+
----------------
chandlerc wrote:
> Are both of these really useful for debugging? We already have a flag that controls whether the pass is enabled or not.
It is actually put there for my own conveniency when debugging. I will remove them.  


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


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


================
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());
----------------
chandlerc wrote:
> 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.
Ok.


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


================
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) {
----------------
chandlerc wrote:
> 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.
Ok, I will do some refactoring based on RecursivelyDeleteTriviallyDeadInstructions. Another motivation is that I need to update MSSA while deleting instruction.  We don't have callback to remove MemoryAccess when we delete memory instruction.




================
Comment at: lib/CodeGen/MemAccessShrinking.cpp:1051-1053
+  bool MadeChange = true;
+  bool EverMadeChange = false;
+  while (MadeChange) {
----------------
chandlerc wrote:
> 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?
It uses the same iterative pattern as CodeGenPrepare, but maybe the iterative pattern in InstCombine is clearer -- only one Changed variable is used there. I will wrap a single pass into a function. I probably rename tryShrinkOnInst to tryShrinkOnFunc and use it to wrap a single pass.  Existing tryShrinkOnInst is simple enough so I can inline its content into tryShrinkOnFunc. 


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


Repository:
  rL LLVM

https://reviews.llvm.org/D30416