This is an archive of the discontinued LLVM Phabricator instance.

[Constant Hoisting]: Hoist Constant GEP Expressions.
Needs ReviewPublic

Authored by hassnaa-arm on Jun 9 2023, 9:20 AM.

Details

Summary
  • By default enable consthoist-gep flag.
  • We saw a 6% improvement on SPEC17 povary and improvements on some internal workloads.
  • There is a little information on why this is disabled by default. Are there specific targets/workloads for which this leads to regressions?

Diff Detail

Event Timeline

hassnaa-arm created this revision.Jun 9 2023, 9:20 AM
Herald added a project: Restricted Project. · View Herald Transcript
hassnaa-arm requested review of this revision.Jun 9 2023, 9:20 AM

I don't recall the details of this, but I think we just didn't spend the time to do the work necessary to enable by default.

For a lot of these regression tests, it isn't obvious why you're passing consthoist-gep=false.

llvm/test/CodeGen/RISCV/hoist-global-addr-base.ll
38 ↗(On Diff #529988)

Regression

290 ↗(On Diff #529988)

Regression.

This was a code-size opt for an internal workload a couple years ago... we didn't excessively tested to enable by default.

  • Avoid hoisting in following cases:
    • The cost of the const expr is 'free' for the target.
    • The expr can be folded into a legal addressing mode for the target.
    • The expr has only single use, and it's not inside a loop.
  • Now there are no regressions.
efriedma added inline comments.Aug 16 2023, 10:52 AM
llvm/lib/Transforms/Scalar/ConstantHoisting.cpp
436

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.

674

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.

889

I'm not sure I follow how you're proving "not inside a loop" here.

hassnaa-arm added inline comments.Aug 17 2023, 10:18 AM
llvm/lib/Transforms/Scalar/ConstantHoisting.cpp
436

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

674

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.

889

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.