Page MenuHomePhabricator

[Constant Hoisting] Hoisting Constant GEP Expressions

Authored by zzheng on Aug 28 2018, 5:44 PM.



Leverage existing logic in constant hoisting pass so that constant gep expressions
sharing the same base global variable are transformed into the form of
[Base + Offset], where Base is a constant gep and Offset is an integer.

This transformation can effectively reduce constant pool entries in the backend.

Diff Detail


Event Timeline

zzheng created this revision.Aug 28 2018, 5:44 PM
efriedma added inline comments.Aug 28 2018, 6:19 PM
81 ↗(On Diff #162999)

Could use a comment explaining what ConstExpr and ConstInt represent.

110 ↗(On Diff #162999)

Could use a comment explaining what BaseExpr and BaseInt represent.

111 ↗(On Diff #162999)

I'm sort of confused by the usage of BaseTy; why does the type of the base matter? It just gets bitcast away later, anyway.

412 ↗(On Diff #162999)

getIntImmCodeSizeCost returns either zero or one on ARM... so it's always less than SizeCost?

745 ↗(On Diff #162999)

Please use bitcast+GEP+bitcast, instead of ptrtoint/inttoptr.

zzheng updated this revision to Diff 163165.Aug 29 2018, 1:00 PM

Unfortunately bitcast + gep + bitcast will only change the constant GEP expr from multi-dimension to single dimension, and it's still a constant pool entry after lowering.

zzheng marked 4 inline comments as done.Aug 29 2018, 1:01 PM
efriedma added inline comments.Aug 29 2018, 3:48 PM
407 ↗(On Diff #163165)

This still seems weird; we only hoist if the cost is less than 3?

zzheng added inline comments.Aug 29 2018, 4:13 PM
407 ↗(On Diff #163165)

We want to hoist when cost of hoisting is smaller than cost of not-hoisting.

Currently a constant GEP whose base pointer is a GV, is lowered to a CP entry. The initial value of Cost reflects cost of loading this CP entry.

If loading from CP is always more expensive than computing it by <base + offset>, perhaps we can get rid of this if condition?

efriedma added inline comments.Aug 29 2018, 4:27 PM
407 ↗(On Diff #163165)

Making it unconditional is probably okay.

Well, we might not want to hoist if the offset is so small we aren't actually getting any benefit. (e.g. loading from [r0] and [r0, #4] isn't really better than loading from [r0, #8] and [r0, #12]).

zzheng updated this revision to Diff 163603.Aug 31 2018, 3:57 PM

Updated version reflects discussion with Eli.

Added AArch64 llc test.

Turns out one of our internal changes is bloating code size on thumbv6m. Will upstream a follow up patch address that.

zzheng marked 4 inline comments as done.Aug 31 2018, 4:08 PM
zzheng added inline comments.
407 ↗(On Diff #163165)

This patch tries to change from loading a group of CP entries to loading one CP entry and loading the reset by <Base + Offset>.

However, that's the case on our internal tree. We have a patch that lowers constant GEPs to CP entries

A few more comments with minor tweaks. Otherwise looks good.

182 ↗(On Diff #163603)

Dead declaration.

192 ↗(On Diff #163603)

Don't use default arguments for functions like this... it's more confusing than helpful.

732 ↗(On Diff #163603)

You probably should specify the address-space here, to the same address-space as Ty.

788 ↗(On Diff #163603)

Could you add an assertion here assert(ConstExpr->isCast()), so it's clear this matches the code in collectConstantCandidates?

zzheng updated this revision to Diff 163605.Aug 31 2018, 4:52 PM
zzheng marked 5 inline comments as done.

Addressed Eli's comments.

efriedma accepted this revision.Aug 31 2018, 4:59 PM


733 ↗(On Diff #163605)


This revision is now accepted and ready to land.Aug 31 2018, 4:59 PM
This revision was automatically updated to reflect the committed changes.