[PATCH] D47475: [Local] Make KDominatesJ required for combineMetadata.

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: lib/Transforms/Utils/Local.cpp:2086
       LLVMContext::MD_dereferenceable_or_null};
-  combineMetadata(K, J, KnownIDs);
+  combineMetadata(K, J, KnownIDs, KDominatesJ);
 }
----------------
efriedma wrote:
> Is it actually correct to use the same metadata list if KDominatesJ is false?
> 
> It's undefined behavior if a nonnull load actually produces null.  So preserving nonnull metadata on a hoisted load requires proving that the hoisted load never produces null (separately from proving the load is safe to hoist).
Actually, I guess none of the existing callers actually do that sort of hoisting.

But combineMetadataForCSE needs better documentation for when, exactly, it's correct to use.


https://reviews.llvm.org/D47475