Page MenuHomePhabricator

[Loop-Vectorize] Emit more context in remarks for optimization record This extends the reportVectorizationFailure helper function with more information emitted after setExtraArgs. This avoids cluttering the compiler frontend remarks with low...
Needs RevisionPublic

Authored by hnrklssn on May 7 2020, 4:24 AM.

Details

Summary

...-level details, while still allowing advanced users to get more context without activating debug output.
There are currently many different cases that result in identical remarks about the vectorizer not understanding the loop control flow, and this change aims to disambiguate those cases.

Diff Detail

Event Timeline

hnrklssn created this revision.May 7 2020, 4:24 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 7 2020, 4:24 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
hnrklssn updated this revision to Diff 262633.May 7 2020, 6:16 AM

Updating D79564: [Loop-Vectorize] update test cases to accept new ouput

This extends the reportVectorizationFailure helper function with more information emitted after setExtraArgs. This avoids cluttering the compiler frontend remarks with low...

fhahn added subscribers: thegameg, anemet, fhahn.

Thanks for working on improving this!

Could you add some tests to check the additional context?

One thing we have to be a bit careful about is to not provide too much information tied to LLVM IR, as this can be confusing to users when viewing the remarks in the original source (which has no references to the IR). IIUC you are using allowExtraAnalysis as a proxy for whether the IR info should be included. Not sure what the current practice is in other passes, @anemet or @thegameg might have some input there.

An alternative might be to use the debug info to reference the original location in the source language. But there are also scenarios where this might fall apart (e..g if the problematic exits have been added by other passes).

llvm/include/llvm/Transforms/Vectorize/LoopVectorize.h
179

could this be llvm::function_ref?

also, might be good to rename ExtraInfo to something like GetExtraInfo, to have the name indicate that this is a function.

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

things are quite deeply nested here. It might be better to flip the logic in the checks and have

if (T->getSuccessor(i) != Head)
  continue;

Same for T->getNumSuccessors.

Also if you are only interested in iteration over the successor blocks, you could use successors(Latch).

1089

nit: StringREf?

1091

nit: unnecessary {}

1147

Because of the check at line 1137, shouldn't we only reach here if both the latch and exiting block are != nullptr, i.e. wouldn't the exiting block always be used here?

fhahn requested changes to this revision.May 28 2020, 10:18 AM

Marking as requiring changes, to remove from review-list until updated.

This revision now requires changes to proceed.May 28 2020, 10:18 AM