[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:49
   const TargetMachine *TM = nullptr;
+  llvm::SmallVector<CallGraphNode*, 8> SortedNodeList;
 
----------------
llvm:: unnecessary


================
Comment at: lib/Target/AMDGPU/AMDGPUAnnotateKernelFeatures.cpp:189
   }
-
   return false;
----------------
Random whitespace change


================
Comment at: lib/Target/AMDGPU/AMDGPUAnnotateKernelFeatures.cpp:220
+
+  while(!SortedNodeList.empty()) {
+    CallGraphNode *Node =  SortedNodeList.pop_back_val();
----------------
Space before (


================
Comment at: lib/Target/AMDGPU/AMDGPUAnnotateKernelFeatures.cpp:221
+  while(!SortedNodeList.empty()) {
+    CallGraphNode *Node =  SortedNodeList.pop_back_val();
+    Function *Caller = Node->getFunction();
----------------
You aren't using this as a stack, so this is weird. You could just do a range loop on reverse(SortedNodeList)


================
Comment at: lib/Target/AMDGPU/AMDGPUAnnotateKernelFeatures.cpp:338
   for (CallGraphNode *I : SCC) {
-    Function *F = I->getFunction();
+    // Build a list of CallGraphNodes that is sorted by the number of uses
+    if (I->getNumReferences())
----------------
Comment is misleading since you aren't really sorting it. Also still not clear why the number of uses matter. Is it just to find unused functions?


================
Comment at: lib/Target/AMDGPU/AMDGPUAnnotateKernelFeatures.cpp:351
   return Changed;
+
 }
----------------
Extra whitespace change


================
Comment at: test/CodeGen/AMDGPU/uniform-work-group-resursion-test.ll:1
+; RUN: opt -S -mtriple=amdgcn-amd- -amdgpu-annotate-kernel-features %s | FileCheck %s 
+
----------------
Missing amdhsa in triple

Testname spelling resursion


================
Comment at: test/CodeGen/AMDGPU/uniform-work-group-resursion-test.ll:7
+
+; CHECK: define i32 @fib(i32 %n) #[[FOO:[0-9]+]] {
+define i32 @fib(i32 %n) #0 {
----------------
Better check variable name


================
Comment at: test/CodeGen/AMDGPU/uniform-work-group-test.ll:33
+}
+
+attributes #2 = { "uniform-work-group-size"="true" }
----------------
A testcase with the attribute on an external function might also be useful


https://reviews.llvm.org/D50200