[PATCH] D152550: [Constant Hoisting]: Hoist Constant GEP Expressions.

Wed Aug 16 10:52:12 PDT 2023

efriedma added inline comments.

================
Comment at: llvm/lib/Transforms/Scalar/ConstantHoisting.cpp:436
+      TTI->getIntImmCostInst(Instruction::GetElementPtr, 0, Offset, OffsetTy,
+                             TargetTransformInfo::TCK_SizeAndLatency, Inst);
+  if (Cost == TTI::TCC_Free) {
----------------
Hoisting could still be worthwhile even if the offset is "free", but I guess we can likely handle it after isel.

It's not clear to me why you're trying to compute whether the offset is "free" in two different ways.

================
Comment at: llvm/lib/Transforms/Scalar/ConstantHoisting.cpp:674
+        // it will not add value because the address dimention is just <= 3,
+        // which is not complicated address calculation.
+        continue;
----------------
The number of operands of the constant gep shouldn't matter.  Excluding very exotic cases, a constant GEP is always going to represent the address of a global plus a constant offset.  A GEP with more dimensions isn't a fundamentally different computation.

================
Comment at: llvm/lib/Transforms/Scalar/ConstantHoisting.cpp:889
+          if (IP->getParent() == ConstInfo.RebasedConstants[0].Uses[0].Inst->getParent()) {
+            // This means it's only single use for this const expr, and NOT inside a loop.
+            // No need to hoist single use.
----------------
I'm not sure I follow how you're proving "not inside a loop" here.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152550