[PATCH] D50200: AMDGPU: Handle "uniform-work-group-size" attribute
Matt Arsenault via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Nov 6 09:10:54 PST 2018
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
More information about the llvm-commits
mailing list