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

[Nicolai Hähnle via Phabricator via llvm-commits]

nhaehnle added a comment.

I don't like the fact that we're solving problems with such switches.

Fundamentally, more clustering should never hurt from the perspective of the memory system, as long as we really properly cluster by locality. The way it could hurt is by increasing register pressure (and reducing occupancy). So the correct long-term solution is to make the scheduler better at avoiding the register pressure.

I think we can live with this ugliness for now, but I implore you to look at better solutions.

One must-fix inline.



================
Comment at: llvm/lib/Target/AMDGPU/SIInstrInfo.cpp:103
+    "amdgpu-enable-new-mem-ops-cluster-heuristic",
+    cl::desc("Enable new heuristic for computing max mem ops cluster size"),
+    cl::init(false), cl::Hidden);
----------------
foad wrote:
> Is there a more useful description than "new heuristic"? If we're asking people to choose between two heuristics then the fact that one of them is currently "new" isn't really helpful.
The name of the cl::opt variable must match the name of the command-line option.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83626