This is an archive of the discontinued LLVM Phabricator instance.

[LoopInterchange] Add some optimization remarks.
ClosedPublic

Authored by fhahn on Jul 7 2017, 6:56 AM.

Diff Detail

Event Timeline

fhahn created this revision.Jul 7 2017, 6:56 AM
anemet edited edge metadata.Jul 7 2017, 8:17 AM

Thanks for adding this!

lib/Transforms/Scalar/LoopInterchange.cpp
596–600

Based on the experience in the inliner, it's helpful to emit the estimated cost and the threshold for such remarks. Then the user would know how much to lower the threshold, etc. You may want to issue this in inProfitable.

1002

"Unable to analyze loop" is probably a better wording. Alternatively you could emit the specific reasons under currentLimitations().

test/Transforms/LoopInterchange/loop-interchange-optimization-remarks.ll
59

Either have some minimal source line debug info or use the YAML output (-pass-remarks-output) rather than -pass-remarks= which would have the function name in the remark record. You should be able to find some examples for both of these approaches in the testsuite.

fhahn updated this revision to Diff 106018.Jul 11 2017, 6:52 AM
fhahn marked an inline comment as done.

Thanks for the feedback, I've addressed the review comments

fhahn added inline comments.Jul 11 2017, 7:04 AM
lib/Transforms/Scalar/LoopInterchange.cpp
1002

I've added more optimization remarks to currentLimitations().}

fhahn updated this revision to Diff 106218.Jul 12 2017, 8:13 AM

properly initialize OptimizationRemarkEmitterWrapperPass dependency

anemet added inline comments.Jul 12 2017, 4:42 PM
lib/Transforms/Scalar/LoopInterchange.cpp
596–600

Are you planning to address this too?

929–934

Indentation seems off, you may want to clang-format the diff.

test/Transforms/LoopInterchange/loop-interchange-optimization-remarks.ll
59

This is actually easier to read if you include the full YAML record verbatim rather than just part of the line/record.

fhahn updated this revision to Diff 106421.Jul 13 2017, 7:04 AM

Include cost in "not profitable" remark. Add more context to checks.

lib/Transforms/Scalar/LoopInterchange.cpp
596–600

ah yes, of course!

929–934

Agreed that it looks off. When viewing in my editor, the first '<' is aligned with the O though, so that should be OK?

anemet accepted this revision.Jul 13 2017, 9:41 AM

LGTM with one nit below. Thanks!

lib/Transforms/Scalar/LoopInterchange.cpp
813–881

Please use different remark identifier for each remark ("CurrentLimitation" currently). This way we can measure the different failing cases without parsing the text of the remark.

This revision is now accepted and ready to land.Jul 13 2017, 9:41 AM
fhahn updated this revision to Diff 106617.Jul 14 2017, 4:23 AM

Thanks Adam! I added unique identifiers for CurrentLimitations remarks. I will commit this in a bit, when I have more time on hand.

fhahn closed this revision.Jul 15 2017, 6:13 AM