Page MenuHomePhabricator

Added Remarks for Loop Versioning LICM Pass
ClosedPublic

Authored by Deepak_Porwal on Oct 10 2017, 5:44 AM.

Details

Summary

Added Remark messages for Loop Versioning LICM Pass to get the remark messages while using opt-viewer tool.

Diff Detail

Repository
rL LLVM

Event Timeline

Deepak_Porwal created this revision.Oct 10 2017, 5:44 AM
anemet edited edge metadata.Oct 10 2017, 9:05 AM

Thanks for working on this!

Please include diff context for the change. See https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface how to generate the diff.

lib/Transforms/Scalar/LoopVersioningLICM.cpp
410

It's usually useful to output the actual numbers. See how we do that in Inliner.cpp where we include the cost and the threshold.

Also please use the new closure API with emit. Again, see the inliner or https://reviews.llvm.org/D38285.

Deepak_Porwal added inline comments.Oct 11 2017, 12:17 AM
lib/Transforms/Scalar/LoopVersioningLICM.cpp
410

Working on necessary changes.

Out of curiosity would like to know the reason for using new Closure API with emit for some places and not for all places (as in Inliner.cpp).

Deepak_Porwal set the repository for this revision to rL LLVM.

Incorporated changes suggested by Adam

anemet added inline comments.
lib/Transforms/Scalar/LoopVersioningLICM.cpp
410

Out of curiosity would like to know the reason for using new Closure API with emit for some places and not for all places (as in Inliner.cpp).

I only converted a few in order to test the approach. @vivekvpandya is converting most other places.

We should convert all unless the emit call is already guarded by allowExtraAnalysis. There is also one more subtle case that cannot be converted when instead of the passname PrintAll is passed.

421

Number of runtime checks (<number>) exceeds threshold (<threshold>)

498–501

Can this actually return the instruction as well and then we can use that for the location rather than the entire loop.

504–512

Same here if possible.

519

Versioned loop for LICM. Please remove the new line everywhere.

ashutosh.nema edited edge metadata.Nov 5 2017, 10:56 PM

Thanks for working on this !

lib/Transforms/Scalar/LoopVersioningLICM.cpp
404–405

For individual instruction you can dump the remark at this point.

As suggested by Ashutosh added remark to check legality of instruction in function 'legalLoopInstructions' .

Deepak_Porwal marked 5 inline comments as done.Nov 6 2017, 1:40 AM
Deepak_Porwal added inline comments.
lib/Transforms/Scalar/LoopVersioningLICM.cpp
504–512

Not possible here

eastig added a subscriber: eastig.Nov 8 2017, 8:03 AM
eastig added inline comments.
lib/Transforms/Scalar/LoopVersioningLICM.cpp
213–215

What about to make ORE a class member?

400–402

Maybe it's better to use 'Unsafe' instead of 'Illegal'?

441–456

The message reported here is quite tricky. What I understand is: per cent of invariant memory operations in less then threshold per cent.
NV does not support FP values. Maybe it's better to add it instead of truncating 'static_cast<int>(InvariantThreshold)'. Diagnostics in this way do not provide actual information.

501–505

Do we need this remark here because remarks related to legality of instructions have already been emitted?

507–511

Maybe legalLoopMemoryAccesses should emit remarks?

Making ORE a class member and using it to emit remarks for various functions.

Deepak_Porwal marked 2 inline comments as done.Nov 27 2017, 3:42 AM
Deepak_Porwal added inline comments.
lib/Transforms/Scalar/LoopVersioningLICM.cpp
441–456

I am not sure how to support FP value in NV

501–505

We can keep this as it is informing about current loop location for the instruction.

507–511

It's difficult to emit remark from legalLoopMemoryAccesses as we don't have current loop info.

This patch looks OK to me, except the floating point value support in NV (maybe it can be done as a later enhancement once NV supports floating point values)

If @anemet & @eastig is OK you can go ahead.

Thanks,
Ashutosh

anemet added inline comments.Nov 28 2017, 11:10 AM
lib/Transforms/Scalar/LoopVersioningLICM.cpp
441–456

That should be trivial. Take a look at how integers are emitted.

501–505

We shouldn't emit multiple remarks for a single problem, if that is what you mean. If legalLoopInstructions has already emitted something, please don't emit anything here.

If you want to mention the loop in the remark in legalLoopInstructions, you should be able to do that. We can already insert DebugLoc into the stream, there should be examples in other passes.

Alternatively, you can use the loop location for the main DebugLoc of the remark and insert the Instruction via an NV. Opt-viewer would display that as a link to the instruction in the code.

519

Can we mention how many checks we had to insert?

eastig added inline comments.Nov 28 2017, 1:52 PM
lib/Transforms/Scalar/LoopVersioningLICM.cpp
507–511

It's difficult to emit remark from legalLoopMemoryAccesses as we don't have current loop info.

Sorry, don't follow. 'legalLoopMemoryAccesses' is a member function. It has the same access to CurLoop as isLegalForVersioning.

Support for float value in NV

Deepak_Porwal marked 5 inline comments as done.Dec 12 2017, 1:29 AM

The patch looks OK to me if using std::to_string is allowed now.

lib/IR/DiagnosticInfo.cpp
172

There were some build issues with std::to_string especially with old gcc versions. I don't know if all of them have been fixed. There is llvm::to_string to work around the issues.

Changed "std::to_string" To "llvm::to_string" as suggested by Evgeny

Deepak_Porwal marked an inline comment as done.Jan 18 2018, 3:56 AM
eastig accepted this revision.Jan 22 2018, 3:05 PM

LGTM

This revision is now accepted and ready to land.Jan 22 2018, 3:05 PM
Deepak_Porwal closed this revision.Feb 21 2018, 9:09 PM