[PATCH] D63401: SROA: Allow touching addrspacecast with volatile

Mon Jun 17 14:32:13 PDT 2019

reames added a comment.

In D63401#1547122 <https://reviews.llvm.org/D63401#1547122>, @arsenm wrote:

> In D63401#1547107 <https://reviews.llvm.org/D63401#1547107>, @reames wrote:
>
> > In D63401#1546293 <https://reviews.llvm.org/D63401#1546293>, @sanjoy wrote:
> >
> > > I don't have opinions on this, but Philip might have comments around whether this is okay for GC pointers.
> >
> >
> > No opinion GC wise, but mostly because I don't understand the intent of this patch.  What is the desired outcome here?
>
>
> For an equivalent bitcast, the type of the underlying alloca is changed into a nicer type and possibly split into multiple allocas (with volatile accesses). I conservatively added checks in r363462 to avoid using the alloca's natural address space for the new operations. I figure it's safer to not change the address space of volatile accesses, but I don't particularly care about this property. InferAddressSpaces avoids changing volatiles, but that was also my idea.

So, just to say this back to you, this would be treating an addrspacecast more like a bitcast when the source is known to be an alloca?

Honestly, this feels more than a bit suspect to me, but I can't argue against it on a principled basis.  Semantics for addrspacecast are so ill specified that it's hard to say what is and isn't legal in terms of transforms.  Just to be clear, the critical detail here to me is "how do we handle addrspacecast", not "how to do handle volatile".  From what I can tell, the existing code for non-volatiles would already have surprising semantics.  I don't see any need to have *different* semantics for volatile vs non-volatile accesses to an addrspacecast derived pointer.  The asc either had semantics, or it didn't.  Whether an access is later volatile shouldn't change that.

So, if anything, I'd either lean towards a more-aggressive transform which changed the AS for the volatile access, or not splitting through an addrspacecast at all.  The intermediate semantics feel odd.

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

https://reviews.llvm.org/D63401