[PATCH] D133422: [amdgpu] Expand all ConstantExpr users of LDS variables in instructions

Tue Sep 13 09:07:16 PDT 2022

JonChesterfield added a comment.

In D133422#3785882 <https://reviews.llvm.org/D133422#3785882>, @foad wrote:

>> Bug noted in D112717 <https://reviews.llvm.org/D112717> can be sidestepped with this change.
>
> Can you add a regression test for that bug?

I could - would have to check the exact failure mode and write a fairly complicated test case. This code can't fall victim to the same bug, as it emits instructions for each phi constantexpr, and the currently broken code is best deleted if/after this patch lands. I'm not sure the test would be value adding but will write it if necessary.

================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULowerModuleLDSPass.cpp:165
+    // Find all ConstantExpr that are direct users of an LDS global
+    SmallVector<ConstantExpr *> Stack;
+    for (auto &GV : M.globals())
----------------
rampitec wrote:
> Should this be a SetVector?
I don't think so. That would avoid pushing constantexpr that use N distinct global variables N times, but I can't think of one that looks like that. The LDS variables tend to be used by addrspacecast or bitcast nodes at the root. So I think we'd gain nothing from the set behaviour.

If I've missed a case, multiple copies of the same constantexpr here don't affect correctness anyway as there's set semantics further down.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133422/new/

https://reviews.llvm.org/D133422