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

Jon Chesterfield via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Nov 16 00:32:09 PST 2021


JonChesterfield added a comment.

> 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. 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.

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.

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.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109870



More information about the llvm-commits mailing list