This is an archive of the discontinued LLVM Phabricator instance.

[BranchFolder] Skip redundant IMPLICIT_DEFs of subregs
ClosedPublic

Authored by foad on Apr 17 2023, 3:07 AM.

Diff Detail

Event Timeline

foad created this revision.Apr 17 2023, 3:07 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 17 2023, 3:07 AM
foad requested review of this revision.Apr 17 2023, 3:07 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 17 2023, 3:08 AM
foad added a comment.Apr 17 2023, 3:09 AM

This is an alternative to D148438. It is based on a new test test/CodeGen/AMDGPU/resource-usage-agpr-hi16.ll that I have not committed yet.

The motivation is to avoid introducing mentions of registers like $agpr0_hi16 that were never mentioned in the original program.

I also like this solution but as I said on D148438, I don't feel like ResourceUsageAnalysis should have crashed anyway upon seeing a agpr_hi16 so I would argue that both D148438 and this could go in (then just change D148438 so it has an artificial MIR test with agpr_hi16)

llvm/lib/CodeGen/BranchFolding.cpp
865

IMO it's only worth merging the logic if in the end it yields less code/more understandable code. It's just a few lines of code here so I'm fine with either solution as long as this or D148438 goes in soon.

866

small nit: isn't there a way to do this more succinctly with any_of?

foad updated this revision to Diff 514156.Apr 17 2023, 3:54 AM

Use any_of. Rename test.

LGTM but I will leave it up to someone that knows BranchFolding better to approve

foad marked an inline comment as done.Apr 17 2023, 3:57 AM
foad added inline comments.
llvm/lib/CodeGen/BranchFolding.cpp
866

Yes using superregs(). It seems mc_superreg_iterator was intended to replace MCSuperRegIterator but it never happened.

arsenm added a subscriber: arsenm.Apr 18 2023, 4:28 AM

This is another place that should be converted to use regunits. We'd then have to go backwards from live reg units to covering registers

foad marked an inline comment as done.Apr 20 2023, 3:56 AM

This is another place that should be converted to use regunits. We'd then have to go backwards from live reg units to covering registers

Wouldn't it be better to convert basic block liveins to use regunits first, so that there would be no need to go backwards?

This is another place that should be converted to use regunits. We'd then have to go backwards from live reg units to covering registers

Wouldn't it be better to convert basic block liveins to use regunits first, so that there would be no need to go backwards?

Yes, that's another project I've wanted to complete

arsenm accepted this revision.Apr 26 2023, 10:11 AM

I guess this is fine until regunits are adopted

This revision is now accepted and ready to land.Apr 26 2023, 10:11 AM
This revision was landed with ongoing or failed builds.Apr 27 2023, 1:42 AM
This revision was automatically updated to reflect the committed changes.