This is an archive of the discontinued LLVM Phabricator instance.

[IRCE] Use SCEVExpander to modify loop bound
ClosedPublic

Authored by dantrushin on Jan 27 2020, 11:21 AM.

Details

Summary

IRCE pass checks that it can calculate loop bounds by checking
SCEV availability at loop entry. However it is possible that loop
bound SCEV is loop invariant, but instruction used to compute it
resides within loop. In such case adjusting loop bound in preheader
using IRBuilder leads to malformed SSA.
Use SCEVExpander instead to generate proper instructions.

Diff Detail

Event Timeline

dantrushin created this revision.Jan 27 2020, 11:21 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 27 2020, 11:21 AM

Unit tests: pass. 62225 tests passed, 0 failed and 816 were skipped.

clang-tidy: pass.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

Fix clang-format error

Unit tests: pass. 62225 tests passed, 0 failed and 816 were skipped.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

mkazantsev added a comment.EditedJan 30 2020, 2:24 AM

I am a bit worried about that. As far as I remember RightValue needs to be in preheader because it is used in checks that decide whether or not we should go to pre/postloops. Does your case generate them? If yes, please add checks to make it explicit. If not, please construct a similar test with pre/postloop and make sure it passes with the fix. If it doesn't, it means that IRCE is illegal in this situation, or that we need some advanced hoisting for these conditions.

llvm/test/Transforms/IRCE/non-loop-invariant-rhs-instr.ll
5

Please add CHECK-LABEL: test so that we could add more here later (maybe makes sense to rename it to test_01).

mkazantsev added inline comments.Jan 30 2020, 4:45 AM
llvm/test/Transforms/IRCE/non-loop-invariant-rhs-instr.ll
2

Please add run command for new pass manager.

Make sure RightValue is always properly instantiated in preheader;
Updated test;

dantrushin marked 2 inline comments as done.EditedJan 30 2020, 8:07 AM

I am a bit worried about that. As far as I remember RightValue needs to be in preheader because it is used in checks that decide whether or not we should go to pre/postloops. Does your case generate them? If yes, please add checks to make it explicit. If not, please construct a similar test with pre/postloop and make sure it passes with the fix. If it doesn't, it means that IRCE is illegal in this situation, or that we need some advanced hoisting for these conditions.

This is exactly a purpose of this patch - ensure that RightValue is instantiated in preheader.
I added one more check to handle a case when we do not need to increment/decrement it.

Unit tests: pass. 62331 tests passed, 0 failed and 838 were skipped.

clang-tidy: pass.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

LGTM (pls make sure that clang-format is fixed).

LGTM (pls make sure that clang-format is fixed).

Thanks. Could you approve it then? (I fixed clang-format locally already, just did not upload such a trivial change)

This revision is now accepted and ready to land.Feb 4 2020, 6:55 PM
This revision was automatically updated to reflect the committed changes.