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

[Florian Hahn via Phabricator via llvm-commits]

fhahn 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);
 }
----------------
nlopes wrote:
> efriedma wrote:
> > fhahn wrote:
> > > efriedma wrote:
> > > > dberlin wrote:
> > > > > efriedma wrote:
> > > > > > 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.
> > > > > 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).
> > > > I'm not sure how you could usefully interpret the definition of !nonnull as anything other than "if the loaded value is null, the behavior is undefined", given that the optimizer can introduce impossible codepaths.
> > > > Is it actually correct to use the same metadata list if KDominatesJ is false?
> > > 
> > > The default behavior for nonnull metadata did not change if KDominatesJ is false, it only combines nonnull md if both K and J have it.
> > > 
> > > IIUC if combineMetadata is used correctly, propagating nonnull from K to a dominated instr J is OK. 
> > > 
> > > It would not be OK, (1) if K is later hoisted to a place where nonnull is not guaranteed to hold, but moving K is causing the undefined behavior, not combining of the metadata. And (2) if the loaded value might become null between K and J. For that to happen, the value needs to be modified and K would not be a valid replacement for J.
> > > 
> > > Am I missing something? The docs for combineMetadataForCSE state that K  must be able to replace J, but is it worth to spelling it out more explicitly what that means?
> > Yes, it would be better to spell it out.
> Just seconding what Daniel wrote: if a "load !nonnull" is UB if it loads null, then it cannot be hoisted (without dropping the flag).  Instead it could rather yield poison.
> Either is fine; I haven't done any study on what are common uses of !nonnull to be able vote either way.
> While at it, updating LangRef to be more explicit about this case would be very much appreciated :)
I've tweaked the comment, does it make sense now? Thanks Eli for updating the LangRef in D47747


https://reviews.llvm.org/D47475