This is an archive of the discontinued LLVM Phabricator instance.

[LSR] Canonicalize formula and put recursive Reg related with current loop in ScaledReg.
ClosedPublic

Authored by wmi on Nov 16 2016, 6:25 PM.

Details

Summary

After https://reviews.llvm.org/D26429, LSR formula can have multiple SCEVAddRecExprs inside of its BaseRegs. Previous canonicalization will swap the first SCEVAddRecExpr in BaseRegs with ScaledReg. But now we want to swap the SCEVAddRecExpr Reg related with current loop with ScaledReg. Otherwise, we may generate code like this: RegA + lsr.iv + RegB, where loop invariant parts RegA and RegB are not grouped together and cannot be promoted outside of loop. With this patch, it will ensure lsr.iv to be generated later in the expr: RegA + RegB + lsr.iv, so that RegA + RegB can be promoted outside of loop.

Diff Detail

Repository
rL LLVM

Event Timeline

wmi updated this revision to Diff 78301.Nov 16 2016, 6:25 PM
wmi retitled this revision from to [LSR] Canonicalize formula and put recursive Reg related with current loop in ScaledReg..
wmi updated this object.
wmi added reviewers: qcolombet, sanjoy, atrick.
wmi set the repository for this revision to rL LLVM.
wmi added subscribers: llvm-commits, davidxl.
qcolombet added inline comments.Nov 17 2016, 5:56 PM
lib/Transforms/Scalar/LoopStrengthReduce.cpp
384 ↗(On Diff #78301)

While updating the canonical representation, please make sure to update the comment on Formula::BaseRegs.

389 ↗(On Diff #78301)

Shouldn't this be just Scale != 1.
I don't expect we would generate 0 scaled register.
What am I missing?

1090 ↗(On Diff #78301)

This change shouldn't be part of the canonicalize patch IMO.

1273 ↗(On Diff #78301)

Why are we dropping this assert?
That doesn't sound right.

1448 ↗(On Diff #78301)

Ditto.

3094 ↗(On Diff #78301)

So far I wanted the canonicalization process to be thoughtful, meaning, no implicit canonicalization like in this change.
The rationale is that in a lot of cases, we will try to canonicalize formulae that are canonical by construction, i.e., we waste compile time.
This is a trade-off between simplicity of the code and compile time.

Was it too complex to keep it that way?
Indeed, I wouldn't have expected we have to add calls to F.canonicalize, except where you changed something.

What am I saying is that unless the code becomes messy very quickly (or every InsertFormula call gets preceded by a call to canonicalize), I would rather keep it the way it was before.

Note: Aside from compile time, the main reason why I don't like that change is because the API looks strange now. I.e., F is not const anymore.

wmi added a comment.Nov 17 2016, 9:45 PM

Thanks for the comments!

lib/Transforms/Scalar/LoopStrengthReduce.cpp
384 ↗(On Diff #78301)

Ok, I will add the comment.

389 ↗(On Diff #78301)

You are right. Scale will not equal to 0 when ScaledReg != NULL.

1273 ↗(On Diff #78301)

I thought it was guaranteed to be true if we did canonicalization before inserting any formula. But as your comments below, we havn't agreed to do implicit canonicalization.

3094 ↗(On Diff #78301)

What am I saying is that unless the code becomes messy very quickly (or every InsertFormula call gets preceded by a call to canonicalize), I would rather keep it the way it was before.

It is reasonable. I will figure out where the canonicalization is needed now, and see how it looks like.

wmi updated this revision to Diff 78896.Nov 22 2016, 11:00 AM

Address Quentin's comments.

Suprising to me, I verified by running a bunch of tests and found that existing canonicalize calls already ensure all the formulas inserted are canonical so I don't have to add extra canonicalize calls. It is indeed better than adding implicit canonicalization inside of InsertFormula. Thanks for the suggestion!

qcolombet accepted this revision.Nov 28 2016, 5:13 PM
qcolombet edited edge metadata.

LGTM.

Thanks Wei!

This revision is now accepted and ready to land.Nov 28 2016, 5:13 PM
This revision was automatically updated to reflect the committed changes.
anna added a subscriber: anna.May 17 2017, 10:54 AM

This patch caused an assertion failure as filed in https://bugs.llvm.org/show_bug.cgi?id=33077
@wmi: Could you please revert this change?