[PATCH] D41715: AMDGPU: Process amdgpu.uniform on loads

[Nicolai Hähnle via Phabricator via llvm-commits]

nhaehnle added inline comments.


================
Comment at: lib/Target/AMDGPU/AMDGPUAnnotateUniformValues.cpp:143-146
+      if (GEP->hasOneUse()) {
+        setUniformMetadata(GEP);
+        return;
+      }
----------------
This makes no sense to me. amdgpu.uniform is about whether the value could differ between threads in a wave. How often the value is used further downstream is irrelevant for that.


================
Comment at: lib/Target/AMDGPU/AMDGPUAnnotateUniformValues.cpp:155
+             Ptr->getType()->getPointerElementType(), Ptr,
+             ArrayRef<Value*>(Idx), Twine(""), &I);
+    I.replaceUsesOfWith(Ptr, GEP);
----------------
mareko wrote:
> arsenm wrote:
> > You don't need the explicit Twine, you could also preserve the original name
> How to do it without Twine and how to preserve the original name? Thanks.
Just put "" instead of Twine("").

Not sure about preserving the original name here -- we're not transforming an instruction here, we're adding an intermediate instruction.


================
Comment at: test/CodeGen/AMDGPU/amdgpu.uniform.ll:5
+; RUN: llc -march=amdgcn -mcpu=gfx900  -verify-machineinstrs < %s | FileCheck -check-prefix=GCN -check-prefix=VIGFX9 %s
+
+; GCN-LABEL: {{^}}uniform_load:
----------------
mareko wrote:
> arsenm wrote:
> > This test should run the IR pass and check that
> Do you mean running the AnnotateUniformValues pass instead of llc? It looks like the llc test does the job too.
Running opt -amdgpu-annotate-uniform is potentially more stable against random changes in unrelated passes.

In this particular case though, I think using llc is justified, because we do need to test that the uniform annotation actually survives through to codegen.


https://reviews.llvm.org/D41715