This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU] Implement flat scratch init for pal
ClosedPublic

Authored by sebastian-ne on Nov 18 2020, 5:32 AM.

Details

Summary

Extract the scratch offset from the scratch buffer descriptor that is
stored in the global table.

Diff Detail

Event Timeline

sebastian-ne created this revision.Nov 18 2020, 5:32 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 18 2020, 5:32 AM
sebastian-ne requested review of this revision.Nov 18 2020, 5:32 AM
arsenm added inline comments.Nov 18 2020, 7:09 AM
llvm/lib/Target/AMDGPU/SIFrameLowering.cpp
322

You should only need the implicit def of the super reg on the first subreg def

392

This already has the explicit def, so I don't see why this implicit def is added

sebastian-ne marked 2 inline comments as done.

Remove superfluous implicit defs.

rampitec added inline comments.Nov 18 2020, 9:38 AM
llvm/lib/Target/AMDGPU/SIFrameLowering.cpp
320

.addReg() on the next line.

320

Isn't this a first subreg def, so this one shall have superreg def?

sebastian-ne added inline comments.Nov 19 2020, 4:40 AM
llvm/lib/Target/AMDGPU/SIFrameLowering.cpp
320

.addReg() on the next line.

The autoformatter complained about that, shall I do it anyway?

Isn't this a first subreg def, so this one shall have superreg def?

I’m not sure? In the if-branch, TargetHi is defined, in the else-branch the whole TargetReg is defined.

arsenm added inline comments.Nov 19 2020, 8:35 AM
llvm/lib/Target/AMDGPU/SIFrameLowering.cpp
320

The formatter is stupid and should be ignored

Reformat BuildMI

rampitec added inline comments.Nov 19 2020, 9:30 AM
llvm/lib/Target/AMDGPU/SIFrameLowering.cpp
320

I mean in the final ISA TergetLo will be defined first, so probably it is TargetLo should carry implicit def of the super-reg?

sebastian-ne added inline comments.Nov 19 2020, 10:17 AM
llvm/lib/Target/AMDGPU/SIFrameLowering.cpp
320

The ISA looks like this:

s_getpc_b64 s[0:1]    <- Created else-branch before, explicitly defines TargetReg
s_mov_b32 s0, s8      <- This move replaces the lower register (TargetLo)

So I think the first definition the whole register is in the if-else and this move does not need an implicit-def anymore?

rampitec accepted this revision.Nov 19 2020, 10:27 AM
This revision is now accepted and ready to land.Nov 19 2020, 10:27 AM
This revision was landed with ongoing or failed builds.Nov 20 2020, 2:14 AM
This revision was automatically updated to reflect the committed changes.