This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU] Declutter applyPreexistingWaitcnt()
ClosedPublic

Authored by stepthomas on Nov 8 2022, 2:52 AM.

Details

Summary

Declutter applyPreexistingWaitcnt() a little by abstracting the code
that updates the operands to S_WAITCNT and S_WAITCNT_VSCNT instructions
into discrete functions.

Diff Detail

Event Timeline

stepthomas created this revision.Nov 8 2022, 2:52 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 8 2022, 2:52 AM
stepthomas requested review of this revision.Nov 8 2022, 2:52 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 8 2022, 2:52 AM
foad accepted this revision.Nov 8 2022, 3:00 AM

Looks fine with or without nits addressed.

llvm/lib/Target/AMDGPU/SIInsertWaitcnts.cpp
848

Slightly neater to return early if they are equal.

855

I know you've copied it from elsewhere but NamedIdx is a terrible name. How about OpName?

914–916

This could also update the named operand simm16 for consistency. There's no particular rule about whether code like this should look up operand indexes or just hard code them.

This revision is now accepted and ready to land.Nov 8 2022, 3:00 AM
stepthomas added inline comments.Nov 8 2022, 3:14 AM
llvm/lib/Target/AMDGPU/SIInsertWaitcnts.cpp
848

True. I'll change this.

855

That's certainly no worse, I'll update that too.

914–916

That would mean I could just have one "updateIfDifferent()" function, which is neater.

stepthomas updated this revision to Diff 473942.Nov 8 2022, 3:28 AM

Apply review comments.

foad accepted this revision.Nov 8 2022, 3:36 AM
This revision was landed with ongoing or failed builds.Nov 8 2022, 3:51 AM
This revision was automatically updated to reflect the committed changes.