[PATCH] D108158: AMDGPU: Invert AMDGPUAttributor

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

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



================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUAttributor.cpp:341
+      if (F->hasFnAttribute(Attr.second))
+        removeAssumedBits(Attr.first);
     }
----------------
This I don't get.
If F has `no-xyz`, why do we remove the bit? Should we not do `addKnownBits(Attr.first)` instead?


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUAttributor.cpp:376
+          removeAssumedBits(AttrName);
+      }
     }
----------------
Nit: I'd call it AttrMask, not Name, but no strong feelings.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUAttributor.cpp:435
+    return getAssumed() != OrigAssumed ? ChangeStatus::CHANGED :
+                                         ChangeStatus::UNCHANGED;
   }
----------------
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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108158/new/

https://reviews.llvm.org/D108158