This is an archive of the discontinued LLVM Phabricator instance.

[Codegen/Statepoint] Allow usage of registers for non gc deopt values.
ClosedPublic

Authored by skatkov on Apr 3 2020, 1:01 AM.

Details

Summary

The change introduces the usage of physical registers for non-gc deopt values.
This require runtime support to know how to take a value from register.
By default usage is off and can be switched on by option.

The change also introduces additional fix-up patch which forces the spilling
of caller saved registers (clobbered after the call) and re-writes statepoint
to use spill slots instead of caller saved registers.

Diff Detail

Event Timeline

skatkov created this revision.Apr 3 2020, 1:01 AM
reames requested changes to this revision.Apr 6 2020, 1:46 PM

Generally seems like a reasonable direction to me, expect this to converge quickly.

llvm/lib/CodeGen/FixupStatepointCallerSaved.cpp
12

A suggestion on how to rephrase this comment.

The key fact we need to encode is that might be a "late read" of the deopt value up to the moment the call returns. If the register allocator could describe such a fact, it would produce the right form for us. The need to fixup (i.e. this pass) is specifically handling the fact that we can't describe such a late read for the register allocator.

I'm using the term "late read" as an analogy to the "early clobber" concept that does exist.

42

Order of includes

141

You can remove the flag check here, as it's implied from the above.

155

I don't understand why you bother to cache this. I expect the lookup to be fast, am I wrong?

I could see having a local data structure in the sort routine, maybe, but I don't see the value outside of that.

167

llvm::sort?

187

OpsToSpill would be more clear.

219

For a reg used multiple times, you're calling addRegSize multiple times.

233

From the look of it, RegToSlotIdx doesn't need to be a member variable and could instead be an out param of this function, and an in param of the next. Once that's done, it looks like rewriteStatepoint is nearly a free function.

252

I don't know that this line is correct. Don't you want the original register size?

323

To keep the size of this list small in practice, you might want to filter out LiveIn case here. (i.e. side exits will dominate number of calls)

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

Given we're planning to move on from this to gc values, I think we need a more descriptive name.
"use-register-for-deopt-values"?

554–556

Everything in this file with the exception of the flag addition and one new comment is NFC. Can you separate, commit separately, and rebase?

(i.e. the rename and refactor of the RequireSpillSlot logic is NFC)

llvm/lib/CodeGen/TargetPassConfig.cpp
916

Can we just run this unconditionally?

This revision now requires changes to proceed.Apr 6 2020, 1:46 PM
skatkov marked 14 inline comments as done.Apr 7 2020, 2:36 AM
skatkov added inline comments.
llvm/lib/CodeGen/FixupStatepointCallerSaved.cpp
12

I've tried to re-phrase, please take a look in new version.

155

ok, let's get rid of that.

219

Yes, no problem with that, but as soon as I'm eliminating addRegSize it does not make sense anymore.

233

I do not follow what you are trying to reach.
Yes, we can extract rewriteStatepoint from StatepointState and provide as an arguments MI, TRI, TII, MF, MFI, RegToSlotIdx and OpsToSpill.

What is an idea behind this?

252

For FixupSCSExtendSlotSize == false it does not matter (size will be the same)
For true case... I do not have a strong preference. It depends on how runtime will determine the size of the value to read.
Let's change this to original register size.

268

Here as well

323

To be honest I do not see any real reason to reduce the size of this list.
LiveIn are filtered out as a first step of process routine.

If you really want to see this list to be small I can move the logic of filtering out LiveIns from process to here.
One minor disadvantage of it would be to trigger StatepointOpers(&MI).getVarIdx() twice for non LiveIns statepoints.

If you believe moving the logic here is more clear, let me know and I do it.

llvm/lib/CodeGen/SelectionDAG/StatepointLowering.cpp
554–556
llvm/lib/CodeGen/TargetPassConfig.cpp
916

Yes, but it will require to update tests which check pipeline.
I would propose to do it as follow-up.

skatkov updated this revision to Diff 255620.Apr 7 2020, 2:37 AM

Changes according to review.

reames accepted this revision.Apr 7 2020, 11:57 AM

LGTM w/two required comments, and one style suggestion.

Please wait 24 hours in case anyone else has comments before landing.

llvm/lib/CodeGen/FixupStatepointCallerSaved.cpp
195

VisitedRegs isn't used outside this function. Reduce scope to local variable above loop.

233

As a general point, the less "object state" there is, the easier the code is to maintain and understand. Structures which are only used in a subset of member functions tend to be some of the most confusing.

You'd need more arguments to the free function than I first thought, so you can ignore the follow up thought.

llvm/test/CodeGen/X86/statepoint-regs.ll
3

I believe -restrict-statepoint-remat is the default. If so, remove from test.

This revision is now accepted and ready to land.Apr 7 2020, 11:57 AM
dantrushin accepted this revision.Apr 7 2020, 12:13 PM
skatkov updated this revision to Diff 255990.Apr 8 2020, 5:33 AM

Handled comments before landing.

skatkov marked an inline comment as done.Apr 8 2020, 9:43 PM
skatkov added inline comments.
llvm/test/CodeGen/X86/statepoint-regs.ll
3

by default it is false, I will return the option back

skatkov updated this revision to Diff 256185.Apr 8 2020, 10:08 PM

return back option in a test

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptApr 9 2020, 3:13 AM

This patch breaks building LLVM with BUILD_SHARED_LIBS due to a linker error. UseRegsitersForDeoptValues is declared as extern in TargetPassConfig while the definition of the value is in SelectionDAG/StatepointLower.cpp. As the LLVMCodegen target does not link against LLVMSelectionDAG it causes undefined reference errors when linking LLVMCodegen

This patch breaks building LLVM with BUILD_SHARED_LIBS due to a linker error. UseRegsitersForDeoptValues is declared as extern in TargetPassConfig while the definition of the value is in SelectionDAG/StatepointLower.cpp. As the LLVMCodegen target does not link against LLVMSelectionDAG it causes undefined reference errors when linking LLVMCodegen

Thanks, I revert it and re-land after the fix.