Page MenuHomePhabricator

SROA: Allow touching addrspacecast with volatile
Needs ReviewPublic

Authored by arsenm on Jun 17 2019, 4:27 AM.

Details

Summary

If the alloca is accessed through an addrspacecasted pointer, allow
the normal changes on the alloca. Cast back to the original use
address space instead of the new alloca's natural address space.

Diff Detail

Event Timeline

arsenm created this revision.Jun 17 2019, 4:27 AM
sanjoy edited reviewers, added: reames; removed: sanjoy.Jun 17 2019, 8:46 AM
sanjoy added a subscriber: sanjoy.

I don't have opinions on this, but Philip might have comments around whether this is okay for GC pointers.

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?

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.

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.

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