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

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

In D133486#3777778 <https://reviews.llvm.org/D133486#3777778>, @efriedma wrote:

> Do we use the sret attribute for optimization anywhere else?
>
> Since sret has ABI implications, I'd prefer to use sret specifically for those ABI implications, and use other attributes for any other semantic meaning, if possible.  (Maybe we need some stronger form of "dereferenceable"?)

Hm, yes, a separate attribute would be cleaner. I would probably frame this as a `writable` attribute with semantics "The underlying object of the parameter must be writable, otherwise the behavior is undefined". I believe that is sufficient, in that it implies that any location that is dereferenceable is also writable. The dereferenceable bytes might be either implied by a dereferenceable attribute, or just from an observed load, which is exactly what we need for scalar promotion.

That said, I'm not sure adding a new attribute is actually worthwhile, as I didn't have a specific use-case in mind for this patch. I guess a nice side effect of having a `writable` attribute is that it can be applied not only to `sret` parameters, but also to all `&mut` parameters in rust (modulo details). As these are also `noalias`, this would allow scalar store promotion on all `&mut` parameters without unwinding interference.



================
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:
> 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.


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

https://reviews.llvm.org/D133486