[PATCH] D50200: AMDGPU: Handle "uniform-work-group-size" attribute

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: lib/Target/AMDGPU/AMDGPUAnnotateKernelFeatures.cpp:226-228
+      // FIXME: Needs to account for linkage and visibility of the function.
+      // The most conservative way is to set uniform-work-group-size 
+      // attribute as false if the function is visible outside of its module
----------------
aakanksha555 wrote:
> arsenm wrote:
> > You should just deal with this fixme and add a test for it. This should be as easy as checking Callee->mayBeRedefined?
> Did you mean Callee->mayBeDerefined() ?
I think so? I'm not sure what the difference is from isInterposable. This also should check F.hasAddressTaken.


================
Comment at: lib/Target/AMDGPU/AMDGPUAnnotateKernelFeatures.cpp:49
   const TargetMachine *TM = nullptr;
+  SmallVector<CallGraphNode*, 8> SortedNodeList;
 
----------------
Remove Sorted from the name now?


================
Comment at: lib/Target/AMDGPU/AMDGPUAnnotateKernelFeatures.cpp:235
+bool AMDGPUAnnotateKernelFeatures::propagateUniformWorkGroupAttribute(Function &Caller, Function &Callee) {
+  bool Changed = false;
+
----------------
You don't need this, which was the point of putting this into a function 


================
Comment at: lib/Target/AMDGPU/AMDGPUAnnotateKernelFeatures.cpp:243-244
+    } 
+    Changed = true;
+    return Changed;
+  }
----------------
You can just return true directly


================
Comment at: lib/Target/AMDGPU/AMDGPUAnnotateKernelFeatures.cpp:254
+        Callee.addFnAttr("uniform-work-group-size", "true");
+        Changed = true;
+      }
----------------
You can just return true


================
Comment at: lib/Target/AMDGPU/AMDGPUAnnotateKernelFeatures.cpp:258
+      Callee.addFnAttr("uniform-work-group-size", "false");
+      Changed = true;
+    }
----------------
You can just return true


================
Comment at: lib/Target/AMDGPU/AMDGPUAnnotateKernelFeatures.cpp:261-263
+    // If the attribute is absent, set it as false
+    Caller.addFnAttr("uniform-work-group-size", "false");
+    Callee.addFnAttr("uniform-work-group-size", "false");
----------------
Can you avoid adding the attribute if not originally present?


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

https://reviews.llvm.org/D50200