[PATCH] D47854: [LangRef] Clarify semantics of load metadata.

[Hal Finkel via Phabricator via llvm-commits]

hfinkel added inline comments.


================
Comment at: docs/LangRef.rst:7941
+program where the memory location is known to be dereferenceable;
+otherwise, the behavior is undefined.
 
----------------
nlopes wrote:
> efriedma wrote:
> > hfinkel wrote:
> > > nlopes wrote:
> > > > hfinkel wrote:
> > > > > nlopes wrote:
> > > > > > aqjune wrote:
> > > > > > > Hello, I have a minor question - is undefined behavior raised at the point when the load is executed? I wonder whether load with !invariant.load can be hoisted out of a loop (as in Nuno's example), even if the loaded value is actually not invariant.
> > > > > > The way the sentence is written implies that !invariant.load needs to be stripped on hoisting. I grepped around and I saw very few cases where it is used. But I guess we cannot change the semantics to be "it has the same value always" since that would mean that memory region is constant instead, and that's not what we want.
> > > > > > So, I guess UB is fine, but we need to strip it on hoisting.
> > > > > > But I guess we cannot change the semantics to be "it has the same value always" since that would mean that memory region is constant instead, and that's not what we want.
> > > > > 
> > > > > Are you sure? I'm under the impression that this is used to indicate things like loads from constant memories on GPUs. I think that we should support hoisting without stripping for this. Granted, however, it means that there can't be any control dependencies on the validity of the metadata. If we have both kinds of use cases, then we'll need to introduce some way to distinguish them.
> > > > > 
> > > > The uses in clang:
> > > > lib/CodeGen/CGObjC.cpp:        CGM.getModule().getMDKindID("invariant.load"),
> > > > lib/CodeGen/CGObjCMac.cpp:    // Annotate the load as an invariant load iff inside an instance method
> > > > lib/CodeGen/CGObjCMac.cpp:        ->setMetadata(CGM.getModule().getMDKindID("invariant.load"),
> > > > lib/CodeGen/CGObjCMac.cpp:  LI->setMetadata(CGM.getModule().getMDKindID("invariant.load"),
> > > > lib/CodeGen/ItaniumCXXABI.cpp:    // Add !invariant.load md to virtual function load to indicate that
> > > > lib/CodeGen/ItaniumCXXABI.cpp:          llvm::LLVMContext::MD_invariant_load,
> > > > 
> > > > So there's definitely more than GPUs :)   (I didn't investigate these)
> > > > 
> > > > The way it's written right now says that the memory is only constant after the load is executed, but makes no promises before the load is executed. Which makes sense: if the instruction is never executed, how could it constrain the execution?
> > > I don't know much about the Objective C bits, but for the vtbl loads, those are in constant memory. No?
> > > 
> > > > The way it's written right now says that the memory is only constant after the load is executed, but makes no promises before the load is executed. Which makes sense: if the instruction is never executed, how could it constrain the execution?
> > > 
> > > I think that, for loads that we know are into constant memory, we actually to have the guarantee. It means that, by the very act of forming the pointer that might potentially feed the load, we know that the pointer is to constant memory.
> > The current language is "at all points in the program where the memory location is known to be dereferenceable".  I think that includes points before the load, if the optimizer can prove the memory is dereferenceable some other way.
> > 
> > That said, the assertion still only applies along codepaths where the load is guaranteed to be executed, so we still have to strip it for some kinds of hoisting.
> Let me be pedantic here. Right now it says:
> "**If** a load instruction tagged with the ``!invariant.load`` metadata **is executed**, the optimizer may assume the memory location referenced by the load contains the same value at all points in the program (…)"
> 
> There's the requisite that the function must be executed before the memory region is known to be constant.
> 
> This forbids this optimization, for example:
> ```
> %v = load %p
> if (foo) {
>    %v2 = load %p, !invariant.load
>    use(%v2)
> }
> use(%v)
> 
>   =>
> 
> %v = load %p, !invariant.load
> if (foo) {
>    use(%v)
> }
> use(%v)
> ```
> 
> Unless foo=true, the program is not claiming that the memory at %p is constant.
> If you really want to mark a region as being constant for the whole program, it sounds more like a property of the pointer (constant) rather than a local property of an instruction that may or may not be executed.
I agree with you, as currently worded we'd need to strip it. My impression, however, is that it's actually generated for loads to pointers we know are only in constant memory.

Nevertheless, I've convinced myself that we need to strip it anyway. We'll need to indicate these constant memory loads in some other way for best optimization. For example, if I have something like this:

  void *x = ...;
  if (foo()) {
    auto *y = (class_w_vtbl *) x;
    y->do_something();
  } else {
    long *y = *(long **) x;
    long z = y[some_offset];
  }

the vtbl load in the first branch is the same as the first load in the second branch, and then the second load in the second branch could easily be the same load as the "invariant" load from the first branch. While language semantics will prevent us from generating loads to vtbl pointers for objects known to have them, this is not true in the face of arbitrary type casts.


Repository:
  rL LLVM

https://reviews.llvm.org/D47854