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

[Andrea Di Biagio via Phabricator via llvm-commits]

andreadb added a comment.

Currently, we set 'SawStore' if there is at least one 'mayStore' instruction in the scheduling region.

However, we could provide a much more accurate information about where 'mayStore' instructions are in the code.
For example, 'BuildInstOrderMap' could populate a (ordered) map '<order_number, MI*>', where MI is a 'mayStore' instruction, and 'order_map' is the instruction number in the sequence.

Later on, you could query that map to check if it is safe to move a load from loc_B to loc_A. If the map contains a store with an order_number in range [loc_A, loc_B], then you know that it is not safe to move it.

This is just an idea for a future improvement.



================
Comment at: lib/CodeGen/LiveRangeShrink.cpp:135
+        if (MI.hasUnmodeledSideEffects() && Next != MBB.end())
+          BuildInstOrderMap(Next, IOM);
+        continue;
----------------
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.


================
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())) {
----------------
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).


================
Comment at: lib/CodeGen/LiveRangeShrink.cpp:192-200
+        // Find MI's debug value following MI.
+        MachineBasicBlock::iterator EndIter = std::next(MI.getIterator());
+        if (MI.getOperand(0).isReg())
+          for (; EndIter != MBB.end() && EndIter->isDebugValue() &&
+                 EndIter->getOperand(0).isReg() &&
+                 EndIter->getOperand(0).getReg() == MI.getOperand(0).getReg();
+               ++EndIter)
----------------
It would be nice to have a STATISTIC that counts the number of instructions reordered by this pass.


https://reviews.llvm.org/D32563