[PATCH] D133486: [LICM] Consider sret as writable object

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/LICM.cpp:1899
 
-  // TODO: Also handle sret.
   if (auto *A = dyn_cast<Argument>(Object))
+    return A->hasByValAttr() || A->hasStructRetAttr();
----------------
efriedma wrote:
> reames wrote:
> > nikic wrote:
> > > reames wrote:
> > > > If you want to be a bit more aggressive here, I believe that every dereferenceable argument satisfies this requirement.  Note that this is specific to the deref globally semantic, not the defer at point semantic.  Since the split on that never got through review, you probably don't want to rely on that.
> > > > 
> > > > You could directly implement the "at point" semantics here - which would be safe - by using dereferenceability in combination with !Value::canBeFreed.  We do have precedent for this in several places already.  
> > > > 
> > > > 
> > > I don't think dereferenceable is sufficient here, because it only guarantees that loads don't trap, it makes no statement about stores. From LangRef:
> > > > This attribute may only be applied to pointer typed parameters. A pointer that is dereferenceable can be loaded from speculatively without a risk of trapping.
> > I really don't think that's a reasonable reading of the spec.  The attribute effects whether memory is dereferenceable (i.e. will not fault).  Having dereferenceability depend on the type of access is insane.  I read that as being an attempt at explaining the semantics, not a restriction on them.  
> > 
> > It would disallow existing transformations such as hoisting a store to a probably dereferenceable and thread local location out of an if block.  (i.e. flattening as done in simplify-cfg for e.g. results of allocations)
> Read-only memory is commonly available to programs in the form of constant globals in LLVM IR.  Stating that constants are dereferenceable doesn't seem that strange?
The dereferenceable attribute as currently used definitely does not imply writability -- e.g. clang itself marks const references (which might point to readonly const globals) as dereferenceable. Other frontends do the same (e.g. rust uses it for non-mut references).

The background here is that many optimizations want to speculate loads, and dereferenceable with current semantics is sufficient for that. Very few optimizations want to speculate stores -- I believe this LICM optimization and the SimplifyCFG store merge optimizations may well be the only ones in LLVM. These optimizations go out of their way to prove that the location is both writable and that inserting the write is thread-safe.

> It would disallow existing transformations such as hoisting a store to a probably dereferenceable and thread local location out of an if block. (i.e. flattening as done in simplify-cfg for e.g. results of allocations)

We already don't perform this optimization unless we know the location is also writable. I just checked the code, and it's actually limited to allocas (if there is no guaranteed-to-execute preceding store). We could expand that to the same logic used here in LICM, but it would still need to check writability as a separate predicate.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133486/new/

https://reviews.llvm.org/D133486