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

Tue Nov 16 00:53:18 PST 2021

hsmhsm added a comment.

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

>> I personally think that you just want to avoid this patch from getting approved, though I do not know the real reason
>
> I'm mistrusting of this patch because it has been landed repeatedly and broke amdgpu openmp every time. That's the 'real reason', every time you land this is breaks openmp and wastes a bunch of my time picking up the pieces.

This is not really true, openmp test fails because this patch exposes some other bug - either in openmp or in amdgpu backend.  We should fix that bug instead of blaming this patch.

> I think you required D110257 <https://reviews.llvm.org/D110257> to land before it might work this time around and that only went in a few hours ago.

D110257 <https://reviews.llvm.org/D110257> landed on Nov 10th, not few hours ago, Please cross-check.

> What I'd have liked to see as part of this patch is runtime tests that show that it works now, given at all points in time  we ran into tests crashing with 'error: memory access' or similar.

If you really wanted this, we could have collaborated internally in a meaningful manner. By the way, if you just wanted thorough openmp testing before approving this patch, why did you again mark it as "needs revision"? After my ping here (which was last week on 10th), you could have approached me to help performing thorough openmp testing.

> Abandoning this optimisation also works from my perspective, 
> but leaving the transform as dead code in trunk indefinitely isn't OK. We need to either make the pass work (for which IR-only tests have repeatedly proven insufficient) or delete it from the tree.

Inform it to AMD technical team, and take further action accordingly.

> I can't find anything on https://llvm.org/docs/CodeReview.html that explicitly rules out your strategy of removing reviewers when they request changes. Maybe it hasn't come up before, I guess I'll ping llvm-dev for clarification.

No comment.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109870