This is an archive of the discontinued LLVM Phabricator instance.

[CodeGen] Additional Register argument to storeRegToStackSlot/loadRegFromStackSlot
ClosedPublic

Authored by cdevadas on Nov 24 2022, 4:30 AM.

Details

Summary

With D134950, targets get notified when a virtual register is created and/or
cloned. Targets can do the needful with the delegate callback. AMDGPU propagates
the virtual register flags maintained in the target file itself. They are useful
to identify a certain type of machine operands while inserting spill stores and
reloads. Since RegAllocFast spills the physical register itself, there is no way
its virtual register can be mapped back to retrieve the flags. It can be solved
by passing the virtual register as an additional argument. This argument has no
use when the spill interfaces are called during the greedy allocator or even the
PrologEpilogInserter and can pass a null register in such cases.

Diff Detail

Event Timeline

cdevadas created this revision.Nov 24 2022, 4:30 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 24 2022, 4:30 AM
cdevadas requested review of this revision.Nov 24 2022, 4:30 AM

This is a better and a stable approach than D138515.
The use case of the extra argument can be found in D124196. See the implementations SIInstrInfo::storeRegToStackSlot and SIInstrInfo::loadRegFromStackSlot.

Ping. Much appreciate if this can be reviewed.

arsenm added inline comments.Dec 9 2022, 10:06 AM
llvm/include/llvm/CodeGen/TargetInstrInfo.h
1063–1068

State what VReg is before stating why it's here

cdevadas updated this revision to Diff 482018.Dec 12 2022, 1:00 AM

Addressed the review comment.

arsenm accepted this revision.Dec 13 2022, 3:09 PM
arsenm added inline comments.
llvm/include/llvm/CodeGen/TargetInstrInfo.h
1064

Still think the phrasing could be improved. How about something like:

If \p SrcReg is being directly spilled as part of assigning a virtual register, \p VReg is the virtual register being assigned.

This revision is now accepted and ready to land.Dec 13 2022, 3:09 PM
cdevadas updated this revision to Diff 482890.Dec 14 2022, 9:18 AM

Fixed the comment appropriately.

I will wait for a couple of days before I upstream it to see if there is any comment from others.

This revision was landed with ongoing or failed builds.Dec 16 2022, 10:26 PM
This revision was automatically updated to reflect the committed changes.