This is an archive of the discontinued LLVM Phabricator instance.

[LV] Inform about exactly reason of loop illegality
ClosedPublic

Authored by psamolysov on May 23 2019, 6:28 AM.

Details

Summary
Currently, only the following information is provided by LoopVectorizer
in the case when the CF of the loop is not legal for vectorization:
LV: Can't vectorize the instructions or CFG
   LV: Not vectorizing: Cannot prove legality.
But this information is not enough for the root cause analysis; what is
exactly wrong with the loop should also be printed:
LV: Not vectorizing: The exiting block is not the loop latch.
Also the information which loops were simplified and
eligible for vectorization will be generated when the 'debug-only'
argument is set to 'loop-vectorize'.

Diff Detail

Event Timeline

psamolysov created this revision.May 23 2019, 6:28 AM
fhahn added a reviewer: fhahn.May 23 2019, 6:37 AM
fhahn added a subscriber: fhahn.

I like the direction of this! Some comments inline. Also, would it be possible to add a test, checking for the expected messages?

llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp
602

Please unify this debug message with the one at line 606. Having both seems redundant.

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
1435

Not sure how helpful this message is. Won't the LV debug output mention the loops it is working on?

7586

Not sure how helpful this information on its own is.

psamolysov marked an inline comment as done.

As fhahn commented, the message on line 602 repeats one on line 606, remove the last.

psamolysov marked 2 inline comments as done.May 23 2019, 6:58 AM
psamolysov added inline comments.
llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp
602

Thank you very much! The second message is removed.

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
1435

Not all loops (for example, outer loops) can be selected as supported and I think it could be helpful (as least for persons not familiar deeply with the vectorization mechanism) to know which loops are selected to be checked on legality and vectorized and which are not.

7586

I agree, it could be not important which loops were simplified, but simplification can change the CFG and it could be not obvious why the pass is processing not the same IR as it might be expected. But if you or any other reviewers don't need this message, I'll remove it.

psamolysov marked 2 inline comments as not done.May 23 2019, 6:59 AM

@fhahn good idea about tge test, I need some time to add it.

The tests have also been added to the diff.

fhahn added a comment.May 24 2019, 3:50 AM

Thanks for adding the test. I think it would be easiest to move this along if you would split off the 2 changes in llvm/lib/Transforms/Vectorize/LoopVectorize.cpp while waiting on additional feedback for those.

llvm/test/Transforms/LoopVectorize/legal_loop_config.ll
1 ↗(On Diff #201154)

I think -disable-output would be slightly better than -o /dev/null.

psamolysov updated this revision to Diff 201196.EditedMay 24 2019, 4:47 AM

@fhahn Thank you for the idea, I've reverted the changes in the LoopVectorizer.cpp file and used -disable-output in the tests.

While we are looking at this, I'd like to discuss how to make these things easier. I think there a merit in using a utility function that takes three strings, something along the lines of
the following pseudo code:

void reportVectorizationFailure(StringRef ORETag, StringRef OREMsg, StringRef DebugMsg) {
      LLVM_DEBUG(dbgs(), concatenate("LV: Not vectorizing: ", DebugMsg));
      ORE->emit(createMissedAnalysis(ORETag), OREMsg);
}

Should reduce/eliminate the chances of missing ORE/DEBUG message (i.e., one exists but not the other) and less boilerplate typing. This is just a direction check. I'm not asking
@psamolysov to do this as part of this patch. Better be a separate NFC patch if pursuing.

@hsaito I agree, to have a function to report about an error is a good idea.

I'm new in LLVM community, so what does NFC mean? Should I close this review and open a new one or you mean just to upload a new diff for comments?

@hsaito I agree, to have a function to report about an error is a good idea.

I'm new in LLVM community, so what does NFC mean? Should I close this review and open a new one or you mean just to upload a new diff for comments?

NFC stands for Non Functional Change. No, your review is good here. I'm just throwing an idea for another separate patch, trying to get other people's opinion,
while I was thinking why LLVM_DEBUG wasn't added here when ORE->emit was added.

@hsaito @fhahn I've introduced the 'reportVectorizationFailure' function, a NFC patch is open for review: D62478 This patch stands unchanged and if you accept it, ready to be applied.

fhahn accepted this revision.May 28 2019, 9:54 AM

LGTM. Please take a look at the remaining test nits before committing

llvm/test/Transforms/LoopVectorize/legal_loop_config.ll
1 ↗(On Diff #201196)

nit: Maybe something like loop-legality-checks.ll would be a slightly more descriptive name.

llvm/test/Transforms/LoopVectorize/legal_preheader_check.ll
1 ↗(On Diff #201196)

maybe fold this test into llvm/test/Transforms/LoopVectorize/legal_loop_config.ll, it would make sense to have the legality checks in a test file together.

This revision is now accepted and ready to land.May 28 2019, 9:54 AM

@fhahn thank you! I've updated the diff - consolidate the legality checking tests in one file: loop-legality-checks.ll
I have no commit access, so could you or someone else lend the patch?

fhahn added a comment.May 28 2019, 1:06 PM

Will do, thanks!

This revision was automatically updated to reflect the committed changes.