[PATCH] D14398: Strip metadata when speculatively hoisting instructions

[Sanjoy Das via llvm-commits]

sanjoy accepted this revision.
sanjoy added a comment.
This revision is now accepted and ready to land.

With the nits addressed, this LGTM.


================
Comment at: lib/Analysis/LoopInfo.cpp:125
@@ +124,3 @@
+  // There is possibility of hoisting this instruction above some arbitrary
+  // condition. Any metadata defined on it can be control dependant on this
+  // condition. Conservativelly stip it here so that we don't give any wrong
----------------
Nit: "dependent"

================
Comment at: lib/Analysis/LoopInfo.cpp:126
@@ +125,3 @@
+  // condition. Any metadata defined on it can be control dependant on this
+  // condition. Conservativelly stip it here so that we don't give any wrong
+  // information to the optimizer.
----------------
Nit: Conservatively strip

Fix these spelling errors elsewhere too.

================
Comment at: lib/Transforms/Scalar/LICM.cpp:675
@@ -674,1 +674,3 @@
 
+  // Metadata can be dependant on the condition we are hoisting above.
+  // Conservativelly strip all metadata on the instruction.
----------------
This is clearly a step in the right direction.

However, I think we can be more precise than this:  (unless I'm missing something) if we justified hoisting the operation using `isGuaranteedToExecute` and not via `isSafeToSpeculativelyExecute`, I think we can keep the metadata.  Perhaps something like that could be done in a separate patch?


Repository:
  rL LLVM

http://reviews.llvm.org/D14398