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

[hassnaaHamdi via Phabricator via llvm-commits]

hassnaa-arm 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) {
----------------
efriedma wrote:
> 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.
Hi Eli, thanks for looking at the patch :)) 

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

The other way you are referring to is the cost calculation at line: 451, correct ?
If yes, then that cost calculation represents the cost of the hoist (computing the GEP by  <base + offset>.

But here, this cost calculation asks if the GEP free/should NOT be hoisted or not.
So, some targets avoid hoisting GEP, so they consider its cost as FREE, ex: RISCV.
Does that make sense ? :'D 


================
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;
----------------
efriedma wrote:
> 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.
This condition represents a case like this:
```
getelementptr i8, ptr @src, 0, 1
getelementptr i8, ptr @src, 0, 2
```
Gep hoisting will generate replacing instructions like this:
```
%const = bitcast ptr getelementptr i8, ptr @src, i32 0, i32 1) to ptr
%mat_gep = getelementptr i8, ptr %const, i32 1
additional bitcast for casting %mat_gep to ptr
```
So hoisting here didn't add a big benefit, instead it caused additional instruction.


================
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.
----------------
efriedma wrote:
> I'm not sure I follow how you're proving "not inside a loop" here.
Because the hoisting occurs in the same block if all uses are in the same block, and if that block is not a loop,
and IP is the instruction that the BitCast for base constant will be instead before,
So If I proved that the rebased constant is in the same block as the place where the base constant will be inserted into (block of the IP), given that there is single use (single rebased constant) then that means the use is NOT inside a loop.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152550