[PATCH] D24171: Refactor LICM to expose canSinkOrHoistInst to LoopSink pass.

[Chandler Carruth via llvm-commits]

chandlerc added a comment.

In https://reviews.llvm.org/D24171#532277, @danielcdh wrote:

> 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.


================
Comment at: llvm/trunk/include/llvm/Transforms/Utils/LoopUtils.h:473-475
@@ -471,1 +472,5 @@
 
+/// canSinkOrHoistInst - Return true if the hoister and sinker can handle this
+/// instruction. If SafetyInfo is not nullptr, check if the instruction can
+/// execute speculatively.
+bool canSinkOrHoistInst(Instruction &I, AAResults *AA, DominatorTree *DT,
----------------
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 ...".

================
Comment at: llvm/trunk/lib/Transforms/Scalar/LICM.cpp:435-437
@@ -438,4 +434,5 @@
 
 /// canSinkOrHoistInst - Return true if the hoister and sinker can handle this
 /// instruction.
 ///
+bool llvm::canSinkOrHoistInst(Instruction &I, AAResults *AA, DominatorTree *DT,
----------------
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*.


Repository:
  rL LLVM

https://reviews.llvm.org/D24171