[PATCH] D109870: [AMDGPU] Enable the pass "amdgpu-replace-lds-use-with-pointer"

Tue Sep 21 07:17:54 PDT 2021

arsenm added a comment.

In D109870#3012287 <https://reviews.llvm.org/D109870#3012287>, @JonChesterfield wrote:

> Hi Roman, Nicolai,
>
> This is an IR transform which assumes alloca are contiguous within the entry block. Mahesha is presenting your replies to the linked thread as community approval for miscompiling code where that assumption does not hold. Somewhat indirectly, this pass will replace load/store of a addrspace(3) global variable with load/store from an uninitialised address if there happens to be a call to a function using said variable in the middle of the alloca in the entry block.
>
> I won't speak for either of you here. I will say that I consider using your replies to a tangentially related question as authority to fast track a known broken IR pass to be inconceivable behaviour.

This pass is an optimization. We do not need anything here for correctness and can do what's simplest. The fact that out of entry block allocas may be broken is totally unrelated to this patch, and all discussion related to correctness is a distraction here

================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUReplaceLDSUseWithPointer.cpp:188
+  // alloca is encountered, and return immediate previous instruction (in
+  // backward direction) as the split point.
+  Instruction *findEntryBlockSplitPoint(Function *K) {
----------------
jdoerfert wrote:
> There is no reason to assume allocas are clustered in any way in the entry block. That is certainly never the case.
It's not required, but it's good enough for optimization purposes. Nothing should be trying to produce IR with allocas in any way

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109870