[PATCH] D32563: Add LiveRangeShrink pass to shrink live range within BB.

[Dehao Chen via Phabricator via llvm-commits]

danielcdh added inline comments.


================
Comment at: lib/CodeGen/LiveRangeShrink.cpp:94-95
+
+bool LiveRangeShrink::runOnMachineFunction(MachineFunction &MF) {
+  MachineRegisterInfo &MRI = MF.getRegInfo();
+
----------------
andreadb wrote:
> You should early exit if this function must be skipped.
> 
> ```
> if (skipFunction(*MF.getFunction())
>   return false;
> ```
> 
> We want to be able to 'opt-bisect' this pass.
> Also, this pass shouldn't be run on optnone functions.
skipFunction added. Thanks for the comment.

This will not be run in optnone as in:

  if (getOptLevel() != CodeGenOpt::None)
    addPass(&LiveRangeShrinkID);



================
Comment at: lib/CodeGen/LiveRangeShrink.cpp:135
+        if (MI.hasUnmodeledSideEffects() && Next != MBB.end())
+          BuildInstOrderMap(Next, IOM);
+        continue;
----------------
skatkov wrote:
> Why don't you refresh SawStore here?
Not sure if I understand the comment. SawStore is updated in MI.isSafeToMove. Do you mean that I can remove the previous "SawStore = BuildInstOrderMap", and set SawStore's initial value to "false"?


================
Comment at: lib/CodeGen/LiveRangeShrink.cpp:178
+        // Skip all the PHI instructions
+        while (I != MBB.end() && I->isPHI())
+          I = std::next(I);
----------------
andreadb wrote:
> I think that you should also skip debug vaue instructions.
> 
> The loop at line 192 works under the assumption that a MI's DBG_VALUE always follows directly after it. This is also suggested by your comment at line 189.
> 
> If you don't skip debug values, then you potentially break the above-mentioned assumption.
Done. Great catch!


https://reviews.llvm.org/D32563