[PATCH] D45151: [LICM] Hoisting invariant.group loads

[Hal Finkel via Phabricator via llvm-commits]

hfinkel added a comment.

You say that you don't yet know if this is profitable yet. Do you have reason to believe that it might not be profitable (e.g., some example where it seems like we might want to constrain the behavior)? We almost never favor keeping metadata over doing other transformations, so I think it's worth being explicit about our reasoning here. Just saying "I don't know yet" is probably not sufficient, as we could say that about nearly all metadata, and in that case, change the default logic.



================
Comment at: lib/Transforms/Scalar/LICM.cpp:1052
   // Metadata can be dependent on conditions we are hoisting above.
-  // Conservatively strip all metadata on the instruction unless we were
-  // guaranteed to execute I if we entered the loop, in which case the metadata
-  // is valid in the loop preheader.
+  // Conservatively strip all metadata on the instruction unless we can keep it.
   if (I.hasMetadataOtherThanDebugLoc() &&
----------------
This now sounds fairly tautological. How about saying, "Except when we can prove the metadata independent of any such conditions, strip it." (instead of "Conservatively strip all metadata on the instruction unless we can keep it.")


================
Comment at: lib/Transforms/Scalar/LICM.cpp:1055
       // The check on hasMetadataOtherThanDebugLoc is to prevent us from burning
       // time in isGuaranteedToExecute if we don't actually have anything to
       // drop.  It is a compile time optimization, not required for correctness.
----------------
isGuaranteedToExecute -> canKeepMetadata


Repository:
  rL LLVM

https://reviews.llvm.org/D45151