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

[Dehao Chen via Phabricator via llvm-commits]

danielcdh added a comment.

Thanks for the reviews!



================
Comment at: lib/CodeGen/LiveRangeShrink.cpp:135
+        if (MI.hasUnmodeledSideEffects() && Next != MBB.end())
+          BuildInstOrderMap(Next, IOM);
+        continue;
----------------
andreadb wrote:
> skatkov wrote:
> > danielcdh wrote:
> > > 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"?
> > My understanding is that if you meet that MI has side effect and it is a barrier you will not move later instruction through this one. SawStore is also a kind of barrier and you cannot move instruction which intersects with the store instruction. After you set the new barrier you can reset SawStore. Say if you saw store before new barrier and there is no stores after the barrier you are free to move instruction which do not intersect with stores. However if SawStore has been set to true once I do not see the place where it can be to reset to false. Do I miss anything?
> > 
> > However the overall logic around SawStore is not clear to me. SawStore is set in BuildInstOrderMap. So it detects where there is a store in the tail. It would prevent us re-ordering instruction (we move instruction up always) even if store is located later. Is it expected?
> You are right. When BuildInstrOrderMap() is called, SawStore should be updated too. Otherwise we risk to prevent the reordering of loads in the new scheduling region.
I've updated the logic to update SawStore on the fly and reset to false when an instruction with unmodeled side effect is met.


================
Comment at: lib/CodeGen/LiveRangeShrink.cpp:81-85
+bool BuildInstOrderMap(MachineBasicBlock::iterator Start, InstOrderMap &M) {
+  bool SawStore = false;
+  M.clear();
+  unsigned i = 0;
+  for (MachineInstr &I : make_range(Start, Start->getParent()->end())) {
----------------
andreadb wrote:
> We can early exit from the loop in BuildInstOrderMap as soon as we encounter an instruction with unmodeled side effects.
> 
> An instruction with unmodeled side effects is essentially a scheduling barrier.
> We don't need to keep numbering instructions after a scheduling barrier because the algorithm would force a recomputation of OrderMap (see line 138).
That would actually make the compile time slower by adding additional check on the frequent path (BuildInstOrderMap), as instructions with unmodeled side effects is extremely rare in real programs.


https://reviews.llvm.org/D32563