The intention was to limit the size of a cluster to 16 bytes, but it was
testing NumLoads which is the number of loads/stores in the cluster
*before* adding the current one. So in fact it would have happily
clustered two dwordx4 loads, making a total cluster size of 32 bytes.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Unit tests: pass. 61291 tests passed, 0 failed and 736 were skipped.
clang-tidy: fail. Please fix clang-tidy findings.
clang-format: pass.
Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml
Don't we *want* clusters that large, and even larger?
Consider some code that loads an array-of-structures (AoS). We really want to cluster that as aggressively as possible, to increase the chance of lowest-level cache hits on successive instructions? I would say the method is *very* inexact :)
The comment talks about not wanting to drive register pressure up too much. That's a legitimate concern, but this approach here seems to be quite wrong to me. The scheduler ought to track register pressure properly, and that's where the knowledge about whether to break clusters based on register pressure should be.
Maybe :-)
Consider some code that loads an array-of-structures (AoS). We really want to cluster that as aggressively as possible, to increase the chance of lowest-level cache hits on successive instructions? I would say the method is *very* inexact :)
The comment talks about not wanting to drive register pressure up too much. That's a legitimate concern, but this approach here seems to be quite wrong to me. The scheduler ought to track register pressure properly, and that's where the knowledge about whether to break clusters based on register pressure should be.
shouldClusterMemOps runs as part of a DAG mutation to insert "cluster" edges in the DAG, before we try to schedule the DAG. So yes you could argue that shouldClusterMemOps should aspire to cluster as much as possible, and it should be up to the the scheduler proper to worry about register pressure, and decide whether or not to schedule those mem ops contiguously. The scheduler does already track register pressure, so it should be able to make this kind of decision, but I don't know how well it works in practice.
Since D73292 landed this is equivalent to fixing the FIXME at the end of SIInstrInfo::shouldClusterMemOps. I'll rebase it accordingly. As for whether it's actually needed, I think only benchmarks can tell, and I don't have any up-to-date numbers at the moment.
Abandoning as there have been other changes to SIInstrInfo::shouldClusterMemOps since I wrote this patch.
Extra parens around LHS