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

[Daniel Berlin via Phabricator via llvm-commits]

dberlin 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).
Yeah, i'm pretty sure there are some pre-existing bugs here, and this looks like it may be one of them.

I admit to wondering what we actually expect here.
The langref just says "The existence of the !nonnull metadata on the instruction tells the optimizer that the value loaded is known to never be null. "

Does that mean "never to be null when performed in this place in the instruction stream" or just "never be null. "

If the former, then yeah, when hoisting, we need to prove nonnullness or drop it.
If the latter, then hoisting would be fine.

I wonder how this is getting used in practice (IE are people treating it like the former/latter, and if the latter, do they expect hoisting to drop non-nullness).


https://reviews.llvm.org/D47475