[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:91-93
+std::list<NodeWrapper*> node_list;
+std::map<CallGraphNode*, NodeWrapper*> node_map;
+
----------------
No globals


================
Comment at: lib/Target/AMDGPU/AMDGPUAnnotateKernelFeatures.cpp:229
 
+bool AMDGPUAnnotateKernelFeatures::processUniformWorkGroup(CallGraphSCC &SCC) {
+  bool Changed = false;
----------------
I don't understand the point of anything this function is doing. You're just copying the same information that's already present in the SCC into a slightly different structure


================
Comment at: lib/Target/AMDGPU/AMDGPUAnnotateKernelFeatures.cpp:232
+
+  for (CallGraphSCC::iterator it = SCC.begin(), end = SCC.end(); it != end; ++it) {
+    //Create NodeWrappers for the nodes and append them to node_list
----------------
Range loop


================
Comment at: lib/Target/AMDGPU/AMDGPUAnnotateKernelFeatures.cpp:235
+    if ((*it)->getNumReferences()) {
+      NodeWrapper *temp = new NodeWrapper;
+      temp->Owner = *it;
----------------
This leaks and should also be unnecessary


================
Comment at: lib/Target/AMDGPU/AMDGPUAnnotateKernelFeatures.cpp:242
+      //Update node_map with reference to the NodeWrapper for all the nodes
+      node_map.insert(std::pair<CallGraphNode*, NodeWrapper*>((*it), temp));
+
----------------
std::make_pair


================
Comment at: lib/Target/AMDGPU/AMDGPUAnnotateKernelFeatures.cpp:251
+
+bool AMDGPUAnnotateKernelFeatures::propagteAttribute() {
+  bool Changed = false;
----------------
Typo propagte


================
Comment at: lib/Target/AMDGPU/AMDGPUAnnotateKernelFeatures.cpp:372
+ 
+  //propagate uniform-work-group-attribute
+  Changed |= processUniformWorkGroup(SCC);
----------------
Comment should be capitalized, have a space after // and be punctuated


================
Comment at: test/CodeGen/AMDGPU/uniform-work-group-nested-function-calls.ll:21
+}
+
+attributes #2 = { "uniform-work-group-size"="true" }
----------------
Should also test a recursive loop


https://reviews.llvm.org/D50200