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

Matt Arsenault via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Jun 18 12:34:57 PDT 2019


arsenm added a comment.

In D63401#1547136 <https://reviews.llvm.org/D63401#1547136>, @reames wrote:

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


In the original patch for addrspacecast support in SROA, the only questions that came up were surrounding overflow behavior which is currently not specified in the LangRef. As for interchanging accesses in different address spaces, I think that is clearly allowed. The original and result pointer still need to access the same memory, so changing the address space or eliminating the access should be OK. I think volatile is the question here because different address spaces may have different caching behavior or something along those lines


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

https://reviews.llvm.org/D63401





More information about the llvm-commits mailing list