This is an archive of the discontinued LLVM Phabricator instance.

Refactor LICM to expose canSinkOrHoistInst to LoopSink pass.
AbandonedPublic

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

Details

Summary

LoopSink pass shares the same canSinkOrHoistInst functionality with LICM pass. This patch exposes this function in preparation of https://reviews.llvm.org/D22778

Diff Detail

Event Timeline

danielcdh updated this revision to Diff 70097.Sep 1 2016, 4:39 PM
danielcdh retitled this revision from to Refactor LICM to expose canSinkOrHoistInst to 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:39 PM
danielcdh added a reviewer: danielcdh.

This is also obvious change, commit without review.

This revision is now accepted and ready to land.Sep 1 2016, 4:39 PM
danielcdh closed this revision.Sep 1 2016, 4:39 PM
This revision was automatically updated to reflect the committed changes.
chandlerc edited edge metadata.Sep 1 2016, 5:39 PM

This is also obvious change, commit without review.

As with the other patch, please avoid submitting without review after you ask folks to do the review...

Generally, I don't know that it makes sense to split this one out (and I tried to make that clear in the other thread). This is now a public function without any other users, which we try to avoid. Anyways, I would have avoided splitting this one out... Anyways, more a note for the future. I don't think there are really substantive changes needed here.

That said, I *did* have some comments about this part of the change that I hadn't made yet... Again, this is minor stuff, but stuff that indicates the change might not be across the "obvious" bar yet.

llvm/trunk/include/llvm/Transforms/Utils/LoopUtils.h
473–475 ↗(On Diff #70098)

Please use the modern doxygen format for this comment.

Unrelated and really minor nit pick: I also would suggest using the indicative mood rather than the imperative for API level comments although this isn't super important. The difference would be "Returns true ..." rather than "Return true ...".

llvm/trunk/lib/Transforms/Scalar/LICM.cpp
435–437 ↗(On Diff #70098)

Please don't leave a duplicate doxygen comment for this API when adding a declaration in a header with its own comment. They have a tendancy to grow stale.

In fact, I'll point out that it is *already stale*.

danielcdh reopened this revision.Sep 1 2016, 7:15 PM

Oops, I saw people approving their own patch and commit without approval from others and thought it's common practice. If that's not the case, I apologize.

Revert r280429 and r280425 in r280453, and will reopen https://reviews.llvm.org/D24168#532233 to address comments.

This revision is now accepted and ready to land.Sep 1 2016, 7:15 PM
danielcdh abandoned this revision.Sep 1 2016, 7:16 PM

Abondon this revision as it's supposed to be together with the main patch.