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

Mon Sep 12 12:12:15 PDT 2022

efriedma 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();
----------------
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?

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

https://reviews.llvm.org/D133486