This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU] Introduce SIInstrWorklist to process instructions in moveToVALU
ClosedPublic

Authored by skc7 on Mar 29 2023, 9:28 AM.

Details

Summary

This patch introduces SIInstrWorklist to process a worklist of MI in moveToVALU. SIInstrWorklist maintains a vector of MachineInstr for worklist and another vector of MachineInstr for deferred list. Deferred list is used to delay the legalization of instruction.

Currently MBUF instructions are tracked in deferred list and will be processed after all the instructions in Initial worklist are done.

This patch is prerequisite for D141030

Diff Detail

Event Timeline

skc7 created this revision.Mar 29 2023, 9:28 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 29 2023, 9:28 AM
skc7 requested review of this revision.Mar 29 2023, 9:28 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 29 2023, 9:28 AM
skc7 updated this revision to Diff 509572.Mar 30 2023, 1:27 AM

Fix mul.ll test failure.

I guess this is one way to do it

llvm/lib/Target/AMDGPU/SIFixSGPRCopies.cpp
872

Should this avoid being reconstructed for each handled case?

llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
6154–6155

Why did you revert from using a SetVector for this case?

6193–6195

Capitalize Impl. You can also do the refactor to use the implicit function with a different typedef for the wordlist in a precommit

6341

whitespace error

6488–6489

Don't understand why this null check was introduced

llvm/test/CodeGen/AMDGPU/control-flow-fastregalloc.ll
13–23

We don't want checks for all this old style metadata. If you're going to generate the checks, use a triple with a newer default code object. Also convert tests to generated checks in a pre-commit

skc7 updated this revision to Diff 510403.Apr 2 2023, 10:31 PM
skc7 retitled this revision from [WIP] Introduce SIInstrWorklist to process instructions in moveToVALU to [AMDGPU] Introduce SIInstrWorklist to process instructions in moveToVALU.
skc7 added reviewers: arsenm, bcahoon, cdevadas.
skc7 set the repository for this revision to rG LLVM Github Monorepo.

Changes done as per @arsenm review.

skc7 updated this revision to Diff 510404.Apr 2 2023, 10:39 PM
skc7 edited the summary of this revision. (Show Details)

Rebase.

skc7 updated this revision to Diff 510741.Apr 4 2023, 3:58 AM
skc7 set the repository for this revision to rG LLVM Github Monorepo.

Rebase. Update test control-flow-fastregalloc.ll

skc7 added inline comments.Apr 4 2023, 4:11 AM
llvm/lib/Target/AMDGPU/SIFixSGPRCopies.cpp
872

SIInstrWorklist is needed for every moveToVALU call. In lowerVGPR2SGPRCopies method, all the copies are being pushed to worklist and one call to moveToVALU would be made. But in other methods, a new worklist object needs to be created and passed to moveToVALU.

llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
6154–6155

The previous patch was a WIP, I was trying to experiment other things. Now, moved worklists to setVectors

6193–6195

Changed to moveToVALUImpl. This method is tied to this current implementation, where it uses newly created SIInstrWorklist . Not sure, how can it be made into precommit.

6341

Ran clang-format before submitting the patch. Should I move the break below the curly braces?

6488–6489

It was part of WIP patch. Was trying to fix few tests. Removed it.

llvm/test/CodeGen/AMDGPU/control-flow-fastregalloc.ll
13–23

Now updated the check lines which were failing. Reverted from generating automatic check-lines with script.

arsenm added inline comments.Apr 4 2023, 6:50 AM
llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
20

Leftover new include?

6169

Can just return contains();

6186–6191

Add a comment explaining what deferred instructions are

6341

The break should be before the curly brace but clang-format probably doesn't handle moving across a brace

skc7 updated this revision to Diff 510813.Apr 4 2023, 8:29 AM

Changes done as per review by @arsenm

skc7 edited the summary of this revision. (Show Details)Apr 6 2023, 2:38 AM
skc7 added a reviewer: foad.
skc7 set the repository for this revision to rG LLVM Github Monorepo.
arsenm added a comment.Apr 7 2023, 4:41 PM

LGTM with nit

llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
6186–6192

Doesn't explain why these are deferred

arsenm accepted this revision.Apr 7 2023, 4:41 PM
This revision is now accepted and ready to land.Apr 7 2023, 4:41 PM
This revision was landed with ongoing or failed builds.Apr 9 2023, 11:43 PM
This revision was automatically updated to reflect the committed changes.
foad added inline comments.Apr 11 2023, 1:51 AM
llvm/test/CodeGen/AMDGPU/sub.ll
2

Please use more descriptive prefixes: GFX6, GFX8, GFX9.

223–224

Should keep this explanatory comment (unless it is no longer relevant).

skc7 added a comment.Apr 12 2023, 9:51 AM

@foad As per your feedback, changes to sub.ll test have been made and merged upstream : LINK

foad added a comment.Apr 12 2023, 9:52 AM

@foad As per your feedback, changes to sub.ll test have been made and merged upstream : LINK

Thank you.