This is an archive of the discontinued LLVM Phabricator instance.

AMDGPU: Invert AMDGPUAttributor
ClosedPublic

Authored by arsenm on Aug 16 2021, 1:24 PM.

Details

Summary

Switch to using BitIntegerState for each of the inputs, and invert
their meanings.

This now diverges more from the old AMDGPUAnnotateKernelFeatures, but
this isn't used yet anyway.

Diff Detail

Event Timeline

arsenm created this revision.Aug 16 2021, 1:24 PM
arsenm requested review of this revision.Aug 16 2021, 1:24 PM
Herald added a reviewer: baziotis. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a subscriber: wdng. · View Herald Transcript
kuter added a comment.Aug 16 2021, 2:28 PM

I think it makes sense to reverse the attributes.

llvm/lib/Target/AMDGPU/AMDGPUAttributor.cpp
384

I don't think this should always return CHANGED here. I think we should only return CHANGED
if we actually changed a bit.

This code as is, will always return CHANGED if NeedsQueuePtr` is true. Which will make the AbstractAttribute stuck in a loop. Attributor times out after a set number of iterations, but this will waste compile time.

411

Same here ^^^

426

Here too ^^

kuter added inline comments.Aug 16 2021, 2:35 PM
llvm/lib/Target/AMDGPU/AMDGPUAttributor.cpp
367

Rather then doing this we can use BitIntegerState::joinAND()or rahter IntegerBaseState::operator&=() which calls joinAND() internally.

jdoerfert added inline comments.Aug 16 2021, 10:18 PM
llvm/lib/Target/AMDGPU/AMDGPUAttributor.cpp
35

Again, I would suggest an array of structs but I guess it works like this too. Benefit of tying them together is that we don't need the 1 << I trick at the use side but can just use the bit (=enum) itself.

342

And this would become a range loop over the array of structs instead.

384

I recommend to take the state before and return CHANGED if the final state is not the initial one, it's an int we copy and compare so there is little cost.

jdoerfert added inline comments.Aug 17 2021, 11:56 AM
llvm/docs/AMDGPUUsage.rst
914

Btw. this is great. Was missing before and a pain for @kuter .

arsenm updated this revision to Diff 367007.Aug 17 2021, 2:09 PM
arsenm marked 5 inline comments as done.

Use pair/mask. Return state changed, which also fixes recursive function handling

All but one comment are nits. That one we should probably clarify but generally this is otherwise good to go.

llvm/lib/Target/AMDGPU/AMDGPUAttributor.cpp
341

This I don't get.
If F has no-xyz, why do we remove the bit? Should we not do addKnownBits(Attr.first) instead?

376

Nit: I'd call it AttrMask, not Name, but no strong feelings.

435

Nit: consider a lambda for this check, we probably should have a helper in the AA namespace so you can do return AA::getChangeStatus(OrigAssumed, getAssumed()); It is really common by now.

arsenm updated this revision to Diff 368724.Aug 25 2021, 2:19 PM
arsenm marked an inline comment as done.

Fix handling of existing attributes on declarations

This revision is now accepted and ready to land.Aug 26 2021, 8:26 AM
arsenm closed this revision.Aug 26 2021, 6:42 PM
llvm/lib/Target/AMDGPU/AMDGPUAttributor.cpp
435

A lamdba is way too heavy for this, especially since it's just to return a glorified bool