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

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added a comment.

I think you need to split this into a separate loop before the propagate attributes function instead of adding the recursive call at the same time. This is different from the other attributes because it is inferred top down. You should have a first loop over the CallGraphSCC that adds this. Since the CallGraphSCC should have all of the nodes reachable from each other, this should be some set building / checks from there. You shouldn't need to be looking at the instructions inside the functions and looking for specific call sites



================
Comment at: lib/Target/AMDGPU/AMDGPUAnnotateKernelFeatures.cpp:227-228
+
+        if (F.hasFnAttribute("uniform-work-group-size")) {
+          if (F.getFnAttribute("uniform-work-group-size").getValueAsString().equals("false")) {
+            Callee->addFnAttr("uniform-work-group-size", "false");
----------------
You don't need to check if it has the attribute and check the value == false. Just check the value == true


================
Comment at: lib/Target/AMDGPU/AMDGPUAnnotateKernelFeatures.cpp:238
+        //Check for nested function calls
+        Changed |= propagateAttribute(*Callee);
+        } else {
----------------
This shouldn't use recursion. This will fail when there's a recursive call. It also shouldn't be necessary, because callees should be called before callers in an SCC pass


================
Comment at: test/CodeGen/AMDGPU/uniform-work-group-test1.ll:1
+; RUN: opt -S -mtriple=amdgcn-amd- -amdgpu-annotate-kernel-features %s | FileCheck -check-prefix=GCN %s 
+
----------------
GCN check prefix is usually used for ISA check lines. It doesn't really matter, but would require more changes if for some reason in the future an llc run line were added


================
Comment at: test/CodeGen/AMDGPU/uniform-work-group-test1.ll:4-5
+; Test 1
+; GCN: define void @foo() #[[FOO:[0-9]+]] {
+define void @foo() #0 {
+  ret void
----------------
Functions and attribute group variables could use better names (as well as the test file). It would also help to add a comment at the top of each test file for what case this is supposed to be


================
Comment at: test/CodeGen/AMDGPU/uniform-work-group-test5.ll:1
+; RUN: opt -S -mtriple=amdgcn-amd- -amdgpu-annotate-kernel-features %s | FileCheck -check-prefix=GCN %s 
+
----------------
Triple is broken


================
Comment at: test/CodeGen/AMDGPU/uniform-work-group-test5.ll:23
+attributes #1 = { "uniform-work-group-size"="true" }
+attributes #2 = { "" }
+
----------------
This is accepted by the IR parser? I would remove all of these empty attribute groups


https://reviews.llvm.org/D50200