Page MenuHomePhabricator

[SCEV] Consider delinearization pattern with extension with identity factor
ClosedPublic

Authored by chrisj on Jan 22 2016, 4:44 PM.

Details

Summary

The delinearization algorithm did not consider terms which had an extension without a multiply factor, i.e. a identify factor. We lose cases where size is char type where there will no multiply factor.

Diff Detail

Repository
rL LLVM

Event Timeline

chrisj updated this revision to Diff 45768.Jan 22 2016, 4:44 PM
chrisj retitled this revision from to [SCEV] Consider delinearization pattern with extension with identity factor.
chrisj updated this object.
chrisj set the repository for this revision to rL LLVM.
chrisj added reviewers: sanjoy, grosser, mssimpso.
chrisj added subscribers: grosser, sanjoy, mssimpso.
mssimpso added inline comments.Jan 25 2016, 7:46 AM
test/Analysis/Delinearization/terms_with_identify_factor.ll
1 ↗(On Diff #45768)

Hi Chris,

I think you need "REQUIRES: asserts" if you're checking the output of -debug.

grosser accepted this revision.Jan 26 2016, 1:10 AM
grosser edited edge metadata.

Hi Chris,

this patch is clearly correct (in terms of it does not mis-compiling anything) and also beneficial. To be a little bit more sure we do not accidentally regress on test cases that have an sext further outside, I would probably match for the expression sext(SCEVUnknown), specifically. This seems to be the precise pattern we are looking for.

I also tested your patch on 3D arrays/loops and due to SCEV hoisting these checks out the simple matching your propose does not work. It might be interesting to look into handling sext expressions for 3D arrays as well.

Best,
Tobias

This revision is now accepted and ready to land.Jan 26 2016, 1:10 AM
mssimpso resigned from this revision.Feb 19 2016, 10:06 AM
mssimpso removed a reviewer: mssimpso.

Looks like patch was not committed.

Apologies, forget this one. I will update the test with mathew's comment.

chrisj updated this revision to Diff 74315.Oct 11 2016, 6:07 PM
chrisj edited edge metadata.
chrisj removed rL LLVM as the repository for this revision.

Updated the test command and added "REQUIRES: asserts"

chrisj set the repository for this revision to rL LLVM.Oct 11 2016, 6:07 PM

Hi Chris,

I am fine with this patch, but as written in my previous review I would prefer for it to match for the precise pattern sext<SCEVUnknown>, rather than just sext<*> to make it very clear for which pattern we match. If this change is OK for you and still catches the cases you are interested in, just post an updated patch and I will commit it.

chrisj updated this revision to Diff 74605.Oct 13 2016, 5:30 PM

This LGTM. Can you commit this by yourself?

This revision was automatically updated to reflect the committed changes.

OK. I committed this change for you!