[PATCH] D114361: [MachineCSE] Add an option to enable global CSE

[Dave Green via Phabricator via llvm-commits]

dmgreen added a comment.

> RISCV. 
> (And it seems that I need to modify other targets' tests)
>
> The differences(in scope of functions):
>
> - Some loads of immediates are redundant.
> - Some loads of global symbols are redundant.
> - etc.
>
> These redundancies are in nonadjacent(non-local?) blocks , so they can't be eliminated according to `Heuristics #1` in `MachineCSE::isProfitableToCSE`.

OK, that's a good start. I was expected something among the lines of "I have tested RISCV on the llvm test suite or some other large codebase under Oz and it reduced the total codesize by 0.16%".

My experiments on ARM and AArch64 are not as great. This seems to increase codesize more than it reduces it, especially on ARM.  The AArch64 numbers were dominated by one large increase, with some of the smaller cases being smaller. I would be interested in what the tests in-tree showed too.

You might want to check X86 as it's easy to run. If I was making target independent changed like this I would expect to test at least a couple of architecture combos (say, X86 with Arm and AArch64 for 32bit and 64bit variants), and potentially add target overrides where needed. In this case the default should maybe be kept as before, unless we have some evidence this is beneficial across most architectures.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114361