[PATCH] D19493: AMDGPU/SI: Add pass for promoting uniform loads to constant address space

[Matt Arsenault via llvm-commits]

arsenm added inline comments.

================
Comment at: lib/Target/AMDGPU/GCNOptimizeUniformMemOps.cpp:1
@@ +1,2 @@
+//===-- GCNOptimizeUniformMemOps.cpp - Optimize Uniform Loads -------------===//
+//
----------------
I'm not sure Optimize is the right name to use for this. Maybe promote or scalarize?
GCNPromoteUniformLoadsToSMRD?

================
Comment at: lib/Target/AMDGPU/GCNOptimizeUniformMemOps.cpp:22
@@ +21,3 @@
+/// This optimization is safe as long as that the memory pointed to by the
+/// load's pointer has not bee updated by the current kernel.  Writes to
+/// global memory do not update the Scalar Data Cache, so in this case we
----------------
Typo bee

================
Comment at: lib/Target/AMDGPU/GCNOptimizeUniformMemOps.cpp:44
@@ +43,3 @@
+
+// FIXME: This can create globals so should be a module pass.
+class GCNOptimizeUniformMemOps : public FunctionPass,
----------------
Why would this need to create globals?

================
Comment at: lib/Target/AMDGPU/GCNOptimizeUniformMemOps.cpp:84
@@ +83,3 @@
+INITIALIZE_PASS_END(GCNOptimizeUniformMemOps, DEBUG_TYPE,
+                    "GCN Optmize Uniform Mem Ops", false, false)
+
----------------
Typo Optmize

================
Comment at: lib/Target/AMDGPU/GCNOptimizeUniformMemOps.cpp:100
@@ +99,3 @@
+  MemDepResult Dep = MemDep->getDependency(cast<Instruction>(&I));
+  if (Dep.isNonFuncLocal()) {
+    IRBuilder<> Builder(&I);
----------------
Can be early return

================
Comment at: lib/Target/AMDGPU/GCNOptimizeUniformMemOps.cpp:103
@@ +102,3 @@
+    Value *Ptr = I.getPointerOperand();
+    Type *ElTy = cast<PointerType>(Ptr->getType())->getElementType();
+    Type *NewTy = PointerType::get(ElTy,
----------------
there is a getPointerElementType helper

================
Comment at: lib/Target/AMDGPU/GCNOptimizeUniformMemOps.cpp:113-114
@@ +112,4 @@
+bool GCNOptimizeUniformMemOps::runOnFunction(Function &F) {
+  if (F.hasFnAttribute(Attribute::OptimizeNone))
+    return false;
+
----------------
I think there's a new skipFunction test instead

================
Comment at: test/CodeGen/AMDGPU/uniform-mem-opt.ll:2
@@ +1,3 @@
+; RUN: llc < %s -march=amdgcn -mcpu=fiji -verify-machineinstrs | FileCheck --check-prefix=GCN %s
+
+; GCN-LABEL: {{^}}no_memdep:
----------------
There should be a run line / test with just opt. With opt you could write a test with a calls that might write through or capture the pointer


http://reviews.llvm.org/D19493