[PATCH] D24168: Refactor LICM pass in preparation for LoopSink pass.

[Dehao Chen via llvm-commits]

danielcdh reopened this revision.
danielcdh marked an inline comment as done.
danielcdh added a comment.
This revision is now accepted and ready to land.

In https://reviews.llvm.org/D24168#532400, @chandlerc wrote:

> In https://reviews.llvm.org/D24168#532233, @danielcdh wrote:
>
> > 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.


================
Comment at: lib/Transforms/Scalar/LICM.cpp:445-451
@@ +444,9 @@
+                        LoopSafetyInfo *SafetyInfo) {
+  if (!isa<LoadInst>(I) && !isa<CallInst>(I) &&
+      !isa<BinaryOperator>(I) && !isa<CastInst>(I) && !isa<SelectInst>(I) &&
+      !isa<GetElementPtrInst>(I) && !isa<CmpInst>(I) &&
+      !isa<InsertElementInst>(I) && !isa<ExtractElementInst>(I) &&
+      !isa<ShuffleVectorInst>(I) && !isa<ExtractValueInst>(I) &&
+      !isa<InsertValueInst>(I))
+    return false;
+
----------------
chandlerc wrote:
> 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.
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.

================
Comment at: lib/Transforms/Scalar/LICM.cpp:468
@@ -459,3 +467,3 @@
     if (LI->getType()->isSized())
-      Size = I.getModule()->getDataLayout().getTypeStoreSize(LI->getType());
+      Size = LI->getModule()->getDataLayout().getTypeStoreSize(LI->getType());
 
----------------
chandlerc wrote:
> Any particular reason to make this change? It's not necessarily a bad change, but it seems surprising here.
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.


https://reviews.llvm.org/D24168