Ping 3: Add a LOAD_SEQUENCE_POINT ISDOpcode
evan.cheng at apple.com
Mon Dec 9 10:35:23 PST 2013
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.
More information about the llvm-commits