[PATCH] D83626: [AMDGPU/MemOpsCluster] Guard new mem ops clustering heuristic logic by a flag

[Mahesha S via Phabricator via llvm-commits]

hsmhsm added a subscriber: rampitec.
hsmhsm added a comment.

Hi @nhaehnle

I agree with you on your comments (and also with what @rampitec had made eariler).

We landed-up with this heuristic based on various fact to satisfy all the work-load that we investigated. But, now, I am realizing that there is serious loop-hole with this patch as @rampitec pointed out eariler.

If you carefully look into the original (old) heuristic, though, it also says, it is based on clustered bytes, but, the counted num bytes is not very accurate because of the way it is implemented. There is also a fixme comment for it. My initial journey of this patch started - (1) to remove that fixme, by adding necessary support, (2) and, then to fix the performance issues which are internally raised.

But, now, I reliaze that  working on both (1) and (2) simultaneously led to current issues.  So, my goal now is to make sure that the system is stable as eairler after (1). Once that is achieved, then we have 1:1 parity between old heuristic and its new clean-up. Then only, I can think of (2).

I am working on (1) right now, will put it for review once it is ready.  From this angle, this patch is no more required.

I appreciate all the comments made here so far.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83626