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

[Mahesha S via Phabricator via llvm-commits]

hsmhsm added a comment.

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

> Nothing is clear here. I have zero comprehension of what you are trying to do. This pass will take IR that describes a meaningful computation and translate it to one that uses uninitialised memory. You'd rather argue that this is fine than write an 'if' statement to skip a function containing unexpected IR.
>
> I suggest you escalate this to whatever authority you consider appropriate.

No,  actually it is not clear to me your line of argument - Please checkout with community if an llvm ir with  "function call with interleaving static allocas"  is a meaningful ir from llvm transformation and optimization perspective. As I understand it from replies to my email, community thinks that it is broken ir, it should be fixed, even though there is nothing wrong in the ir from correctness perspective, and community expect to place all static allocas before any call.

And, as I also clarified, if we think that in-spite of above expectation, if we want to be cautious about it in this pass, then we can add IF check as I implemented at https://reviews.llvm.org/D109594 and skip running the pass if there is  "function call with interleaving static allocas". Note that you cannot just skip kernels which have this pattern, and continue with other kernels. We should skip the pass, even if one kernel has this pattern. The reason is, consider this situation - Kernel K1 <https://reviews.llvm.org/K1> and K2 <https://reviews.llvm.org/K2> calls function FOO, FOO uses LDS,  K1 <https://reviews.llvm.org/K1> has this pattern, but K2 <https://reviews.llvm.org/K2> does not. In this case, if we skip K1 <https://reviews.llvm.org/K1>, and go with K2 <https://reviews.llvm.org/K2>, then transformation itself will be broken.

Reviewers need to agree on above.

The point I am making here is - I cannot fix whole universe of issues as part of this patch within in a short frame of time. We need to go with a solution which works for us now, and come back it once the pass settles down.

Yes, we need to have internal meeting for better clarification, we cannot go endlessly like this without common conclusion.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109870