[PATCH] D158081: [IR] Add writable attribute

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/test/Transforms/LICM/scalar-promote.ll:889
+define void @writable_cond_store(ptr sret(i32) noalias writable %ptr) {
+; CHECK-LABEL: @writable_cond_store(
 ; CHECK-NEXT:    [[PTR_PROMOTED:%.*]] = load i32, ptr [[PTR:%.*]], align 4
----------------
efriedma wrote:
> nikic wrote:
> > efriedma wrote:
> > > I'm not sure this transform is actually legal without `dereferenceable(4)`?  The way "writable" is specified, no "deferenceable" means no phantom store, so this is introducing a race.
> > `sret(%T)` implies `dereferenceable(sizeof(%T))`. I should probably tweak the wording to not talk about `dereferenceable(N)` specifically but attributes implying dereferenceability in general.
> > 
> > (And we should really drop the type argument from `sret` and make people specify `align` and `dereferenceable` instead...)
> Is that actually the way the compiler is reasoning about it?  Can you add a negative test showing the sret is actually necessary here?
Yes, you were right and this did not reason based on explicit dereferenceability. I've added a test with an insufficient dereferenceable annotation and adjusted the implementation. I've also adjusted the LangRef wording to tie it to explicit dereferenceability.


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

https://reviews.llvm.org/D158081