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

[wangpc via Phabricator via llvm-commits]

pcwang-thead marked an inline comment as done.
pcwang-thead added a comment.

In D114361#3146082 <https://reviews.llvm.org/D114361#3146082>, @lkail wrote:

> `Global` is not a good name here. MachineCSE is working on the whole function, which is global in compiler's terminology, by walking through the DominatorTree.

Can we name it `greedy`?



================
Comment at: llvm/lib/CodeGen/MachineCSE.cpp:55
+static cl::opt<bool>
+    EnableGlobalCSE("enable-global-cse", cl::Hidden, cl::init(false),
+                    cl::desc("Enable CSE on the whole function."));
----------------
jrtc27 wrote:
> Putting this behaviour behind a cl::opt is a great way to ensure it's never used...
LOL.

So is there a better way? Maybe we can add a target hook to enable this?


================
Comment at: llvm/lib/CodeGen/MachineCSE.cpp:462
+  // Enable global CSE when optimizing for size.
+  if (!EnableGlobalCSE && !MI->getMF()->getFunction().hasOptSize() &&
+      TII->isAsCheapAsAMove(*MI)) {
----------------
lkail wrote:
> Why only apply to this heuristics? Since your intention is reducing size, why not always consider profitable if `hasOptSize`?
You are right, my thought were limited.


================
Comment at: llvm/test/CodeGen/RISCV/enable-global-cse.ll:1
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -mtriple=riscv64 --enable-global-cse -verify-machineinstrs < %s | FileCheck %s
----------------
jrtc27 wrote:
> This should be a MIR test that runs just MachineCSE
Thanks, I will update it later.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114361