[PATCH] D60580: [GlobalISel] Enable CSE in the IRTranslator & legalizer for -O0 with constants only

[Aditya Nandakumar via Phabricator via llvm-commits]

aditya_nandakumar added a comment.

Looks mostly good - there are some minor comments from my side.



================
Comment at: llvm/lib/CodeGen/GlobalISel/MachineIRBuilder.cpp:218
   Res = getMRI()->createGenericVirtualRegister(getMRI()->getType(Op0));
-  unsigned TmpReg = getMRI()->createGenericVirtualRegister(ValueTy);
-
-  buildConstant(TmpReg, Value);
-  return buildGEP(Res, Op0, TmpReg);
+  auto Cst = buildConstant({ValueTy}, Value);
+  return buildGEP(Res, Op0, Cst.getReg(0));
----------------
Nit : {} not necessary


================
Comment at: llvm/lib/CodeGen/TargetPassConfig.cpp:1228
 bool TargetPassConfig::isGISelCSEEnabled() const {
-  return getOptLevel() != CodeGenOpt::Level::None;
+  return true;
 }
----------------
Maybe we should add a hook so targets can return the appropriate CSEConfigs for the opt level instead of the core passes making this decision for them?


================
Comment at: llvm/test/CodeGen/AArch64/GlobalISel/legalize-vaarg.mir:18
     ; CHECK: [[C:%[0-9]+]]:_(s64) = G_CONSTANT i64 8
-    ; CHECK: [[GEP:%[0-9]+]]:_(p0) = G_GEP [[LOAD]], [[C]](s64)
+    ; CHECK: [[COPY1:%[0-9]+]]:_(s64) = COPY [[C]](s64)
+    ; CHECK: [[GEP:%[0-9]+]]:_(p0) = G_GEP [[LOAD]], [[COPY1]](s64)
----------------
Did you look why these copies are generated?


================
Comment at: llvm/test/CodeGen/AMDGPU/GlobalISel/legalize-addrspacecast.mir:307
     ; VI: [[C1:%[0-9]+]]:_(p0) = G_CONSTANT i64 0
+    ; VI: [[COPY2:%[0-9]+]]:_(p0) = COPY [[C1]](p0)
     ; VI: [[EXTRACT:%[0-9]+]]:_(p3) = G_EXTRACT [[UV]](p0), 0
----------------
Similar here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60580