Ping 3: Add a LOAD_SEQUENCE_POINT ISDOpcode

[Evan Cheng]

On Dec 6, 2013, at 9:59 AM, Richard Sandiford <rsandifo at linux.vnet.ibm.com> wrote:

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

Ok, I'm fine with the TargetLowering hook then.

Evan

> 
> Thanks,
> Richard
>