This is an archive of the discontinued LLVM Phabricator instance.

AMDGPU: Invert AMDGPUAttributor

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



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.


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.


Same here ^^^


Here too ^^

kuter added inline comments.Aug 16 2021, 2:35 PM

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

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.


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


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

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.


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


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


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

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