This is an archive of the discontinued LLVM Phabricator instance.

AMDGPU/GISel: Factor out AMDGPURegisterBankInfo::buildReadFirstLane
ClosedPublic

Authored by nhaehnle on May 10 2022, 9:20 AM.

Details

Summary

A later change will add a 3rd user, so factoring out the common code
seems useful.

Reorganizing the executeInWaterfallLoop causes some more COPYs to be
generated, but those all fold away during instruction selection.
Generating the comparisons uses generic instructions over machine
instructions now which admittedly shouldn't make a difference
(though it should make it easier to move the waterfall loop generation
to another place).

Diff Detail

Event Timeline

nhaehnle created this revision.May 10 2022, 9:20 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 10 2022, 9:20 AM
nhaehnle requested review of this revision.May 10 2022, 9:20 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 10 2022, 9:20 AM
Herald added a subscriber: wdng. · View Herald Transcript
sebastian-ne accepted this revision.May 12 2022, 9:45 AM
sebastian-ne added a subscriber: sebastian-ne.

Looks good to me

This revision is now accepted and ready to land.May 12 2022, 9:45 AM
kosarev added inline comments.May 13 2022, 12:22 AM
llvm/lib/Target/AMDGPU/AMDGPURegisterBankInfo.cpp
719

Putting this under #ifndef NDEBUG would prevent calling constrainGenericRegister() in no-asserts builds.

arsenm accepted this revision.May 16 2022, 2:52 PM
arsenm added inline comments.
llvm/lib/Target/AMDGPU/AMDGPURegisterBankInfo.cpp
719

Why would you want to do that? The operands always need to be constrained

1017

Doesn't this happen in the constructor? (I do have a patch I Need to pick up to stop reconstructing this in RegBankSelect)

llvm/test/CodeGen/AMDGPU/GlobalISel/image-waterfall-loop-O0.ll
52

This looks like an improvement, but you also seem to have ended up with more instructions inside the loop (I guess this is -O0 so it doesn't really matter either way)

kosarev added inline comments.May 17 2022, 12:23 AM
llvm/lib/Target/AMDGPU/AMDGPURegisterBankInfo.cpp
719

Yep, I misread it.

This revision was landed with ongoing or failed builds.May 25 2022, 9:35 AM
This revision was automatically updated to reflect the committed changes.
nhaehnle added inline comments.May 25 2022, 9:35 AM
llvm/lib/Target/AMDGPU/AMDGPURegisterBankInfo.cpp
1017

You're right, I removed it.

thakis added a subscriber: thakis.May 25 2022, 9:54 AM

Looks like this breaks tests: http://45.33.8.238/linux/76883/step_12.txt

Please take a look and revert for now if it takes a while to fix.