This is an archive of the discontinued LLVM Phabricator instance.

MIR Statepoint refactoring. Part 3: Spill GC Ptr regs.
ClosedPublic

Authored by dantrushin on Jun 11 2020, 5:10 AM.

Details

Summary

Extend FixupStatepointCallerSaved pass with ability to spill
statepoint GC pointer arguments (optionally allowing them on CSRs).
Special handling is required for invoke statepoints, because at MI
level single landing pad may be shared my multiple statepoints, so
we must ensure we spill landing pad's live-ins into the same stack
slots.

Full change set is available at D81603.

Diff Detail

Event Timeline

dantrushin created this revision.Jun 11 2020, 5:10 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 11 2020, 5:10 AM
skatkov added inline comments.Jun 25 2020, 2:26 AM
llvm/lib/CodeGen/FixupStatepointCallerSaved.cpp
53

Add a comment that it is for debugging purpose. Please mention that not specifying this option means no restrictions at all.

127

SO.getNumDeoptArgsIdx() ?

628

If I understand correctly you are processing starting operands and they are defs.
so the condition MO.isDef() is actually exit condition of the loop (we processed all defs).
What else we can see as def? In other words what MO.isReg condition means?

643

don't you want to rephrase it as

unsigned LastStatepointwithRegsIdx = MaxStatepointsWithRegs.getNumOccurrences() ? MaxStatepointsWithRegs : Statepoints.size();

for (MachineInstr *I : Statepoints)
  Changed |= SPP.process(*I, ++NumStatepoints <= LastStatepointwithRegsIdx);
skatkov added inline comments.Jun 25 2020, 2:26 AM
llvm/lib/CodeGen/FixupStatepointCallerSaved.cpp
131

What if deopt args contains gc pointer?

450

in a separate function?

dantrushin marked 6 inline comments as done.Jun 25 2020, 2:50 AM
dantrushin added inline comments.
llvm/lib/CodeGen/FixupStatepointCallerSaved.cpp
53

Will do.

127

Yep.

131

At this point, we can not know.
And your code handles all deopt args uniformly already ;)
Mine adds some more restrictions

450

Ok

628

MO.isDef() asserts that MO.isReg(), so one always must check later before checking former

643

For upstream, maybe.
But since I have downstream changes here, I'd like to keep it that way if you do not mind.

Move copy propagation to separate function;
Simplify statepoint defs handling a bit;

dantrushin marked 5 inline comments as done.Jun 30 2020, 12:30 PM
skatkov added inline comments.Jul 5 2020, 8:38 PM
llvm/lib/CodeGen/FixupStatepointCallerSaved.cpp
55

Still do not see that not specifying this option actually means infinity.
Because it is a bit strange that
"Max number of statepoints allowed to pass GC Ptrs in registers" and default value 0.
So it makes me thinking that by default the behavior is off.

131

This is because I'm sure that GC pointer cannot be on register until your changes.

627

Please double check, that there is not problems with subregs here.

629

If you do not find reg in statepoint you continue to search. For example you found a Reg in previous statepoint, what does it mean?

To me it means that live gc pointer passed through next statepoint and is not mentioned in that statepoint. it is a bug.
I believe you should assert this or stop on the first statepoint.

Providing def as non-first statepoint seems to be a bug,

skatkov added inline comments.Jul 5 2020, 8:50 PM
llvm/lib/CodeGen/FixupStatepointCallerSaved.cpp
123

Do I understand correctly that with your changes ALL GC pointers must be defs?
So why do you need these iterations instead of just taking all defs?

491

what is the reason for this magic?

576

Don't you want to separate reload loads into separate function?
So we'll have:
spill registers
rewrite statepoint
insertReloads/unspill registers

dantrushin marked 5 inline comments as done.Jul 6 2020, 7:03 AM
dantrushin added inline comments.
llvm/lib/CodeGen/FixupStatepointCallerSaved.cpp
123

Strictly speaking, no. Only derived pointers passed in registers.
Are we guaranteed that all base pointers will appear as derived ones too?
If yes, then it is good catch, taking them from defs is simpler (but taking them from operand list instead of def list sounds a bit more natural, IMHO)

131

So nothing has changed.
At this point there is no way to detect deopt pointer which is not in gc list.
ISEL determines what pointers to pass where.
If implementation cannot handle pointer deopt value, not present in gc list, it should not enable it at all or spill all deopt values

491

The reason is that TTI.loadRegFromStackSlot can insert load only before some existing instruction.

dantrushin marked 2 inline comments as done.Jul 6 2020, 7:55 AM
dantrushin added inline comments.
llvm/lib/CodeGen/FixupStatepointCallerSaved.cpp
576

insertReloads uses local vector RegsToReload and MI (statepoint instruction).
To call insertReloads outside of rewriteStatepoint I will have to make that local vector and new statepoint instruction
available to insertReloads().

I don't think that making RegsToReload member variable or something like that:

SmallVector<Register, 8> RegsToReload;
SS.spillRegisters();
MachineInstr *NewStatepoint = SS.rewriteStatepoint(RegsToReload); // out parameter
SS.insertReloads(RegsToReload, NewStatepoint);

will be much cleaner.
But I can do that if you want.

Address review comments and fix clang-format errors

dantrushin marked 5 inline comments as done.Jul 6 2020, 10:50 AM

A pretty close to be done.

Do we need a special tests for this patch?

llvm/lib/CodeGen/FixupStatepointCallerSaved.cpp
123

I'm a bit confused here. What is the difference between derived and based pointer here?
You have an alive gc pointer. It might be relocated == can be changed. So it must be defined as def independent on whether it is a derived one or based one. Do I miss anything here?

If you are doubt and write that it works under assumption I would suggest under debug assert that sets collected by different way are the same.

491

Does it make sense to add an utility function to TTI which after some existing instruction? It looks more natural then this magic.

576

I do not have strong preference here. But separation seems to me makes sense.
At least rename the function rewriteStatepoint and definitely update the comment before the function due to it does additional things except rewriting statepoints.

dantrushin marked 2 inline comments as done.Jul 7 2020, 2:08 AM
dantrushin added inline comments.
llvm/lib/CodeGen/FixupStatepointCallerSaved.cpp
123

GC pointers always occurs in pairs (base, derived).
Only derived pointers are relocated (and so can be tied to defs of statepoint).
If base pointer need to be relocated it will appear as (base, base) pair.

It is not specified if the base pointer must be relocated together with its derived pointer.
At least, this is how I interpret LLVM docs. So I originally wrote it in a way I had no doubts of.

What's the point of having two implementation and comparing them with assert?
Assert is not proof that 'doubtful' implementation is correct.

491

I will have to implement that function in all backends and then spent 6 months or so asking people
to review change which they won't ever use...
I would like to avoid that

I need an answer to the question about test and at least update doc for rewriteStatepoint.

llvm/lib/CodeGen/FixupStatepointCallerSaved.cpp
123

It will help you to catch easily an error in case I was wrong with defs.

491

ok

dantrushin updated this revision to Diff 276082.Jul 7 2020, 8:28 AM

Return back old implementation of collectGCRegs as I've found case
where statepoint defs != gc args;

Refactor insertReloads() into a separate routine.

Will add some tests soon.

Add two simple MIR tests

could you please explain the case due to you need to return back in collectGCRegs implementation?

As I understand a lot of things here are done to workout the case that one catch block can correspond to different invoke instructions. Can you please add the test examine this case?

dantrushin updated this revision to Diff 276470.Jul 8 2020, 9:56 AM

Add (hand crafted) test for shared landing pad;
Slightly change cache handling code to better handle shared landing pads;
Improve debug output;

Changed DerivedPtrMap and moved it to StatepointLowering.h as it only
needed for local gc.relocate processing;
Rebased on tip, rewrote lowerStatepointMetaArgs as been told;
Deleted test mods, will add separate test later;

Restore accidently destroyed review.

Change handling of shared landing pads - now we reserve spill slots
as we go, upon encountering first statepoint jumping to that landing pad.

Slightly modified tests to be less sensitive to unimportant changes
(spill ordering)

Hi Denis, after you re-write the patch in this way, I guess we can do it clearly and simply in terms of reading.

Let's add set of reserved frame indices into FrameIndexesPerSize.

In reset method, you clean this new set.
In resetAndPreFill you just add all the reserved FI into this set.
In getFrameIndex you lookup FI in GlobalIndices and then take the first available FI which is not in Reserved set.

This way we can get away from this implicit assumptions that we reserved first n elements, supposing that they will processed first and so on...

What do you think?

llvm/lib/CodeGen/FixupStatepointCallerSaved.cpp
308

It does not look valid to me. You'll set Line.Index to value corresponding to last processed pair from It.
While you need a max in this case...

628

remove redundant {} now.

Rework slot reservation according to comments.
Add an option to disable copy propagation.

skatkov added inline comments.Jul 17 2020, 12:48 AM
llvm/lib/CodeGen/FixupStatepointCallerSaved.cpp
302

assert that frame slot is reserved? (nobody used it)

353

Can we just track for each EhPad what registers were reloaded instead of this search?

412

I wonder, do we really need to collect GC regs. According to implementation, any register found in operand after some index is considered as GC reg.
So can we just compute this index and use it in this loop.

Do I miss anything?

Add tracking of reloads inserted in landing pads.

dantrushin marked 5 inline comments as done.Jul 17 2020, 8:42 AM

That EHPad reload tracking does not fit very well in existing design. I added it as a separate entity stored in StatepointProcessor and passed to
StatepointState::insertReloads.
Ideally, I would insert all reloads in a single sweep after all statepoints has been processed. But I did not find a clean way to do that without refactoring
(like merging StatepointStates functionality into`StatepointProcessor`).

llvm/lib/CodeGen/FixupStatepointCallerSaved.cpp
412

This is to handle pointers found in both deopt state and gc pointer section uniformly.
To avoid case when same object is passed both in register and in stack slot.

reames added inline comments.Jul 30 2020, 11:30 AM
llvm/lib/CodeGen/FixupStatepointCallerSaved.cpp
563–564

There's an important semantic bug here. Test case and candidate fix can be found in https://reviews.llvm.org/D84964.

While writing the test case for this, I also stumbled across an unrelated functional bug. Filed as https://bugs.llvm.org/show_bug.cgi?id=46916

Please fix the bug Philip mentioned and I think we are ready to land it.

llvm/lib/CodeGen/FixupStatepointCallerSaved.cpp
130

In InstrEmitter I see getStatepointGCArgStartIdx which does exactly the same what you need.
It also contains:

// FIXME: need a better place for this. Put it in StackMaps?

Can you do it in a separate patch (w/o review) and just re-use this utility function here?

393

This overall algorithm is very sensitive to alone EHPad.
Looking into machine verifier, I see that in some cases we can have more than one landing pad.

 if (LandingPadSuccs.size() > 1 &&
  !(AsmInfo &&
    AsmInfo->getExceptionHandlingType() == ExceptionHandling::SjLj &&
    BB && isa<SwitchInst>(BB->getTerminator())) &&
  !isScopedEHPersonality(classifyEHPersonality(F.getPersonalityFn())))
report("MBB has more than one landing pad successor", MBB);

I would suggest to assert that here we have only one landing pad.

  • Mark MMOs for gc register spill slots as load/store ones;
  • Add assert that there is only one EHPad;
dantrushin marked 4 inline comments as done.Aug 3 2020, 12:24 PM
dantrushin added inline comments.
llvm/lib/CodeGen/FixupStatepointCallerSaved.cpp
130

No, it is slightly different.
During its lifetime statepoint meta args change their format few times. From bare %stack.0 form, to stackmap-encoded
version like 1, 8, %stack.0 here and down to frame register format.
This is the primary reason why I didn't put in into StackMaps.cpp - to remain generic, it'll need some kind of
parameter to tell which exact form it is parsing. Which would look ugly, IMHO

skatkov accepted this revision.Aug 4 2020, 2:33 AM

Thanks!

This revision is now accepted and ready to land.Aug 4 2020, 2:33 AM
This revision was landed with ongoing or failed builds.Aug 14 2020, 6:21 AM
This revision was automatically updated to reflect the committed changes.
RKSimon added inline comments.
llvm/test/CodeGen/X86/statepoint-fixup-copy-prop-neg.mir
3

@dantrushin This is failing on EXPENSIVE_CHECKS builds - please can you take a look?

RKSimon added inline comments.Aug 14 2020, 8:05 AM
dantrushin added inline comments.Aug 14 2020, 8:07 AM
llvm/test/CodeGen/X86/statepoint-fixup-copy-prop-neg.mir
3

Thanks. I still have no email from bot, but reproduced it locally.
Will commit fix in a moment

bjope added a subscriber: bjope.Aug 14 2020, 8:12 AM
bjope added inline comments.
llvm/lib/CodeGen/FixupStatepointCallerSaved.cpp
224

Results in "unused variable" werror in noasserts build

475

Results in "unused variable" werror in noasserts build