Page MenuHomePhabricator

[Statepoints] Change statepoint machine instr format to better suit VReg lowering.
ClosedPublic

Authored by dantrushin on Sep 4 2020, 11:46 AM.

Details

Summary

Current Statepoint MI format is this:

STATEPOINT
<id>, <num patch bytes >, <num call arguments>, <call target>,
[call arguments...],
<StackMaps::ConstantOp>, <calling convention>,
<StackMaps::ConstantOp>, <statepoint flags>,
<StackMaps::ConstantOp>, <num deopt args>, [deopt args...],
<gc base/derived pairs...> <gc allocas...>

Note that GC pointers are listed in pairs <base,derived>.
This causes base pointers to appear many times (at least twice) in
instruction, which is bad for us.
The problem is that machine operand tiedness is 1-1 relation, so
it might look like this:

%vr2 = STATEPOINT ... %vr1, %vr1(tied-def0)

Since only one instance of %vr1 is tied, that may lead to incorrect
codegen (see PR46917 for more details), so we have to always spill
base pointers. This mostly defeats new VReg lowering scheme.

This patch changes statepoint instruction format so that every
gc pointer appears only once in operand list, so that they all can
be tied. Additional set of operands is added to preserve base-derived
relation required to build stackmap.
New statepoint looks like this:

STATEPOINT
<id>, <num patch bytes>, <num call arguments>, <call target>,
[call arguments...],
<StackMaps::ConstantOp>, <calling convention>,
<StackMaps::ConstantOp>, <statepoint flags>,
<StackMaps::ConstantOp>, <num deopt args>, [deopt args...],
<StackMaps::ConstantOp>, <num gc pointers>, [gc pointers...],
<StackMaps::ConstantOp>, <num gc allocas>,  [gc allocas...]
<StackMaps::ConstantOp>, <num pairs in gc map>, <base/derived indices...>

Changes are:

  • every gc pointer is listed only once in a flat list, list is prefixed with its length;
  • alloca list is prefixed with its length too;
  • following alloca list is length-prefixed list of base-derived indices of pointers from gc pointer list. Note that indices are logical (number of pointer), not absolute (index of machine operand).

Diff Detail

Event Timeline

dantrushin created this revision.Sep 4 2020, 11:46 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 4 2020, 11:46 AM
dantrushin requested review of this revision.Sep 4 2020, 11:46 AM
dantrushin edited the summary of this revision. (Show Details)Sep 4 2020, 11:50 AM

Can we come up with better format for gc pointer map (base/derived)?
I wish it were possible to have it in separate psude instruction glued with statepoint. Unfortunately, that seems impossible now.

May be we can have compressed that map? Say, allocating 16 bits for each index, one int64_t immediate operand can hold 2 map pairs (4x space win!)

Rebase on tip and use newly added API;
Slightly change GC pointer operand lowering;

skatkov added inline comments.Sep 29 2020, 11:22 PM
llvm/include/llvm/CodeGen/StackMaps.h
232

doc

llvm/lib/CodeGen/FixupStatepointCallerSaved.cpp
372

Add a comment that it is guaranteed (by what?) that all uses with reg has tied def.
May be add and assert function?

llvm/lib/CodeGen/SelectionDAG/StatepointLowering.cpp
611–618

why not to iterate over LoweredGCPtrs?

llvm/lib/CodeGen/StackMaps.cpp
94

To simplify code review you could move this function here as NFC first...

458–459

separate to utility function?

486

separate to utility function?

llvm/test/CodeGen/X86/statepoint-vreg.mir
3

Why it should fail now?

Address most review comments:

  • add documentation to API
  • factor out some code to separate function
  • fix test instead of XFAILing it
dantrushin marked 6 inline comments as done.Sep 30 2020, 10:40 AM
dantrushin added inline comments.
llvm/lib/CodeGen/SelectionDAG/StatepointLowering.cpp
611–618

Because reservePreviousStackSlotForValue takes IR Value, not SDValue

llvm/lib/CodeGen/StackMaps.cpp
486

I tried, but it looked ugly, because it needs to pass too much context back and forth. So I left it as is

dantrushin marked 2 inline comments as done.Sep 30 2020, 12:07 PM
skatkov added inline comments.Sep 30 2020, 7:52 PM
llvm/lib/CodeGen/FixupStatepointCallerSaved.cpp
373

in a time - "new" will become not a new :)
I would propose something like:

Due to statepoint format, ...
llvm/lib/CodeGen/StackMaps.cpp
372

Does it makes sense to document statepoint format here in comment?
Or may be provide a link to place where it is documented?

458–459

now you can remove redundant {}

dantrushin updated this revision to Diff 295493.Oct 1 2020, 1:50 AM

Update statepoint MI description, provide link to it at parseStatepointOperands;
Fix style issues;

dantrushin marked 3 inline comments as done.Oct 1 2020, 1:56 AM
dantrushin retitled this revision from [WIP][Statepoints] Change statepoint machine instr format to better suit VReg lowering. to [Statepoints] Change statepoint machine instr format to better suit VReg lowering..Oct 1 2020, 2:45 AM
dantrushin edited the summary of this revision. (Show Details)
skatkov accepted this revision.Oct 1 2020, 3:01 AM

Please wait for 1-2 days in case Philip wants to look at this patch.

This revision is now accepted and ready to land.Oct 1 2020, 3:01 AM

Denis asked me to take a glance at this. From a high level, the direction seems entirely reasonable. I'll do a pass through code a bit later today, but in general, I don't have any reason to hold this given Serguei has already reviewed.

LGTM as well with minor comments.

In abstract, I'd have preferred you split this into the NFC format change and the functionality change for vreg lowering. I'm not requesting you do so now as the vreg lowering is off by default, so this is effectively NFC for the enabled lowering.

llvm/lib/CodeGen/SelectionDAG/StatepointLowering.cpp
687

minor: I'd suggest adding asserts that both Base and Derived are in the map. They are from above, but local asserts never hurt.

llvm/lib/CodeGen/StackMaps.cpp
416

Again, add an assert index is within bounds.

442

As a follow up, I might separate this into a parseX routine, and a recordStackMapOpers. Given Statepoints never set recordResult, there's quite a bit of divergence in semantics here. This is not a must fix, just an opportunity to clarify the code.

dantrushin updated this revision to Diff 296396.Oct 6 2020, 2:53 AM

Add local asserts

This revision was landed with ongoing or failed builds.Oct 6 2020, 3:56 AM
This revision was automatically updated to reflect the committed changes.