Page MenuHomePhabricator

Refactor LICM pass in preparation for LoopSink pass.
ClosedPublic

Authored by danielcdh on Sep 1 2016, 4:21 PM.

Event Timeline

danielcdh updated this revision to Diff 70092.Sep 1 2016, 4:21 PM
danielcdh retitled this revision from to Refactor LICM pass in preparation for LoopSink pass..
danielcdh updated this object.
danielcdh added reviewers: chandlerc, davidxl.
danielcdh added a subscriber: llvm-commits.
danielcdh accepted this revision.Sep 1 2016, 4:23 PM
danielcdh added a reviewer: danielcdh.

This is an obvious change, I will commit it without review.

This revision is now accepted and ready to land.Sep 1 2016, 4:23 PM
danielcdh closed this revision.Sep 1 2016, 4:24 PM
chandlerc edited edge metadata.Sep 1 2016, 5:29 PM

This is an obvious change, I will commit it without review.

So, in the code review where I asked for the change, I mentioned I had some comments that should be attached to it. So that might signify that it isn't fully obvious...

And once you *ask* for a pre-commit review, it seem much more polite to see it through. Comments inbound shortly.

chandlerc added inline comments.Sep 1 2016, 5:32 PM
lib/Transforms/Scalar/LICM.cpp
449–455

Why this change?

We try to avoid testing isa<T> just before we do a dyn_cast<T> because it duplicates work. So the other arrangement is in many ways much more common.

If you're worried about the performance of this code or just trying to make it more readable what goes where, there are perhaps better patterns that can be used, but I'm not sure which to suggest without understanding the motivation.

464

Any particular reason to make this change? It's not necessarily a bad change, but it seems surprising here.

530–531

Please prefer early return:

if (!SafetyInfo)
  return true;

// ...
return isSafeTo...
danielcdh reopened this revision.Sep 1 2016, 7:40 PM
danielcdh marked an inline comment as done.

This is an obvious change, I will commit it without review.

So, in the code review where I asked for the change, I mentioned I had some comments that should be attached to it. So that might signify that it isn't fully obvious...

Indeed did not notice/understand that comment, sorry about that.

And once you *ask* for a pre-commit review, it seem much more polite to see it through. Comments inbound shortly.

As noted in the other review, I misunderstood that self-approving patch is common-practice, no mean to be rude, please don't take it personally.

lib/Transforms/Scalar/LICM.cpp
449–455

I remember I had a reason for that... But it's a long time ago, I could not think of it now...

I'm doing some more testing without hoisting it, if I still cannot find the justification, I'll revert this change.

464

This is not related to this patch, just found it easier to read thus went ahead with the change. I can back it out if you want.

This revision is now accepted and ready to land.Sep 1 2016, 7:40 PM
danielcdh updated this revision to Diff 70131.Sep 1 2016, 7:42 PM
danielcdh edited edge metadata.

refactor

danielcdh updated this revision to Diff 70180.Sep 2 2016, 9:58 AM

update comment, which will be moved to header later.

hfinkel added inline comments.
lib/Transforms/Scalar/LICM.cpp
523

Why is this not returning a conservative answer?

danielcdh added inline comments.Oct 3 2016, 11:19 AM
lib/Transforms/Scalar/LICM.cpp
523

SafetyInfo will be non-null for LICM, and will be nullptr for LoopSink, which is always safe. Basically, I used this pointer to distinguish between LICM and LoopSink usage to reuse the code as much as possible without introduce new parameter to the API. Let me know if you think that is confusing:

  • Shall I add comment to clarify here?
  • Or shall I simply add a new parameter to indicate if it's for hoisting or sinking?
chandlerc added inline comments.Oct 3 2016, 11:22 AM
lib/Transforms/Scalar/LICM.cpp
523

This is a good question, it may just mean that "SafetyInfo" needs a better name or this needs a comment to explain why this is correct.

Alternatively, we could require that SafetyInfo is always provided.

chandlerc added inline comments.Oct 3 2016, 11:24 AM
lib/Transforms/Scalar/LICM.cpp
523

(sorry, crossing replies)

I think making it more clear that the difference is between *hoisting* and *sinking* would really help. That explains the reason you don't need to call isSafeToExecuteUnconditionally at all.

danielcdh added inline comments.Oct 3 2016, 11:26 AM
lib/Transforms/Scalar/LICM.cpp
523

Do you mean that I add a comment to the early return?

hfinkel added inline comments.Oct 3 2016, 11:27 AM
lib/Transforms/Scalar/LICM.cpp
523

I agree. This makes sense, but please add a comment here, and also a comment in LoopUtils.h for each relevant interface explaining that passing nullptr for SafetyInfo is for sinking.

hfinkel added inline comments.Oct 3 2016, 11:32 AM
lib/Transforms/Scalar/LICM.cpp
523

Yes. The comment before the early return should specifically point out that for sinking we don't need to check isSafeToExecuteUnconditionally.

danielcdh updated this revision to Diff 73314.Oct 3 2016, 11:43 AM

update comments

Commends updated to LICM.cpp. This change does not affect other functions in LoopUtils.h. But when I move canSinkOrHoistInst to LoopUtils.h in the next patch, I'll have the updated comment there too.

hfinkel accepted this revision.Oct 3 2016, 11:52 AM
hfinkel added a reviewer: hfinkel.

LGTM

lib/Transforms/Scalar/LICM.cpp
440

sinking instruction from should be "sinking instructions from" or "sinking this instruction from"

danielcdh updated this revision to Diff 73319.Oct 3 2016, 11:55 AM
danielcdh edited edge metadata.

update comment

chandlerc accepted this revision.Oct 3 2016, 11:56 AM
chandlerc edited edge metadata.

(FWIW, LGTM to me too, thanks!)

Thanks for the reviews!

As this was original proposed by Chandler, I'll hold this until later today and see if he has more comment.

Hal, it would be great if you can also shed some lights on https://reviews.llvm.org/D22778 (I'll do a quick rebase now). Thanks!

danielcdh closed this revision.Oct 3 2016, 12:01 PM