[PATCH] D100296: [WIP] Teach Greedy RA to recognize special case live-through

[Serguei Katkov via Phabricator via llvm-commits]

skatkov added a comment.

In D100296#2684415 <https://reviews.llvm.org/D100296#2684415>, @reames wrote:

> I really really like this idea, but the implementation, not so much.  :)

This is just WIP :) Let's make it better.

> I assume that you'd disable and eventually delete FixupCalleeSaved if this landed right?  (Mostly checking my understanding of the patch intent.)

Yep, the intent is as. However we should be careful with deleting the pass because it is used for all RAs while this fixes only Greedy RA.
In downstream I've checked that if I modify FixupCallerSaved  and insert an assert there that only callee saved are expected then no assert triggered in my testing (not so big to be honest - I probably should check it more).

> Code structure wise, I think this needs to be merged into the existing loop.  If I'm reading the code correctly, you just need to add a special case when we'd otherwise exit the innermost loop.  If *SlotI == LiveI->end and Live->end is a statepoint deopt use, then add the regmask.  (I believe that SlotE is past the end of all possible instructions, and thus we can't have a statepoint there.)

This was my first implementation but unfortunately it is not true.
The critical pieces are the following:

1. Indeed when we exit innermost loop, SlotI is a first slot after current segment and we can check deopt use.
2. After that we advance to next segment which end is strongly bigger then SlotI. As a result if SlotI is an end of next segment we'll skip this segment and will not handle SlotI. The simplest example is LI with two segments and the second one ends with statepoint. We'll skip it.

> You could also consider adjusting the existing code to use a lambda to do the lazy UsableRegs init.  That would be a lot easier to read even for the existing code, and your approach becomes easier to read if you're not duplicating all of the Found logic.

Agreed.

> If my earlier comment didn't parse, I'd suggest extracting the helper as suggested inline and rebasing over the lambda.  I think once most of the extra complexity is out of the way, it'll be easier to see ways to fold the two loops together.



================
Comment at: llvm/lib/CodeGen/LiveIntervals.cpp:922
+    if (MachineInstr *MI = Indexes->getInstructionFromIndex(S.end))
+      if (MI->getOpcode() == TargetOpcode::STATEPOINT) {
+        StatepointOpers SO(MI);
----------------
reames wrote:
> I think you can make this code much more readable by adding a utility along the lines of the following:
> bool hasLiveThroughUse(MachineInstr, Register);
Agreed.


================
Comment at: llvm/lib/CodeGen/LiveIntervals.cpp:930
+              // TODO - add reg mask
+              while (SlotI4SP != SlotE &&
+                     !SlotIndex::isSameInstr(*SlotI4SP, S.end))
----------------
reames wrote:
> Isn't this just a really weirdly spelled expression of getInstructionIndex on SlotIndexes?
The intent here is as follows:
we found that live segment is ended with statepoint instruction and there is a deopt use for this live interval in this statepoint instruction. Now we need to find regmask corresponding to this statepoint. We can do this using two approaches:
1) Iterate over operands of statepoint and find an regmask operand
2) There are two arrays in this method: Slots and Bits. The first array contains slot indexes for which regmask exists. The second array contains regmasks. if we know slot index we should find an index in Slots array and use found index in Bits to get regmask.

The second approach is implemented here.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100296/new/

https://reviews.llvm.org/D100296