Ping 3: Add a LOAD_SEQUENCE_POINT ISDOpcode

[Richard Sandiford]

Evan Cheng <evan.cheng at apple.com> writes:
> On Dec 6, 2013, at 1:24 AM, Richard Sandiford
> <rsandifo at linux.vnet.ibm.com> wrote:
>
>> Evan Cheng <evan.cheng at apple.com> writes:
>>> On Dec 5, 2013, at 3:07 AM, Richard Sandiford
>>> <rsandifo at linux.vnet.ibm.com> wrote:
>>> 
>>>> Hi Evan,
>>>> 
>>>> Thanks for the review.
>>>> 
>>>> Evan Cheng <evan.cheng at apple.com> writes:
>>>>> I'm not very comfortable with adding a generic opcode. Can't you use a
>>>>> target specific intrinsic?
>>>> 
>>>> If the opcode's out, is it still OK to have a TargetLowering hook that
>>>> gets called before atomic and volatile loads, as in the patch below?
>>>> This replaces the getInsertLoadSequencePoints() function from the first
>>>> patch but should be a bit more general (and hopefully a bit more intuitive,
>>>> since it's named after the situations in which it's used).
>>> 
>>> Ideally I'd like to see the code for dealing with these loads in the
>>> target specific isel lowering code. Until we have other targets needing
>>> similar features, it's a bit difficult to understand whether a opcode
>>> and / or target hooks are needed. Do you think that's possible?
>> 
>> I think doing it in LowerOperation() means that either (a) we just
>> insert a serialiasing instruction before the load or (b) we also replace
>> the load with a different representation, to indicate that it's properly
>> protected by a serialising instruction.
>> 
>> The problem with (a) is that the meaning of a volatile load node then
>> depends on whether we're looking at it before or after lowering.  It seems
>> bad to have that kind of phase-dependent interpretation.
>> 
>> The problem with (b) is that, once protected, the load really is just
>> a normal load.  The architecture has many register-memory instructions
>> and I think it would be quite difficult to go through and make sure
>> that everything that handles memory operands is also able to handle the
>> lowered form.
>
> I'm not sure if I completely follow you. But if the concern is to
> distinguish the loads, would it be cleaner to add a property to
> MemSDNode? And is this property related to SynchronizationScope property
> that's already on MemSDNode?

SynchronizationScope is only defined for atomics rather than normal
volatile loads though.  I don't think it would make sense to "lower"
a volatile load to an atomic one.  For one thing (at least AIUI)
all atomic loads are volatile at the DAG level, so in a sense they're
"higher" rather than "lower" than volatile loads.  Also, it's useful to
allow volatile SEXTLOADs and ZEXTLOADs, which I don't think is possible
with atomics.

(FWIW I have a target-local follow-on patch for atomic loads and stores
and it goes the other way: lowering an atomic operation to a normal
volatile one once it has been properly protected.)

It's not really the load instruction itself that's special.  I want
to match the load exactly as it's matched now and just make sure that
a separate instruction is emitted before it in the chain.  For atomic
fences we do that at the IR->DAG boundary, and that seemed like the
cleanest place to handle this case too.

Thanks,
Richard