[PATCH] D108158: AMDGPU: Invert AMDGPUAttributor

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUAttributor.cpp:55
+  "amdgpu-no-workitem-id-z",
+};
 
----------------
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.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUAttributor.cpp:342
+        removeAssumedBits(1 << I);
     }
   }
----------------
And this would become a range loop over the array of structs instead.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUAttributor.cpp:389
+      return ChangeStatus::CHANGED;
     }
 
----------------
kuter wrote:
> 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.
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.


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

https://reviews.llvm.org/D108158