Ping 3: Add a LOAD_SEQUENCE_POINT ISDOpcode

[Evan Cheng]

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?

Evan

> 
>> If not, then target hook might be acceptable. But I haven't had the time
>> to think this through or review your patch fully.
> 
> Thanks.  FWIW, although the patch is quite long, only the first four hunks
> are target-independent:
> 
> diff --git a/include/llvm/Target/TargetLowering.h b/include/llvm/Target/TargetLowering.h
> index 6f643c3..7bc3a52 100644
> --- a/include/llvm/Target/TargetLowering.h
> +++ b/include/llvm/Target/TargetLowering.h
> @@ -2082,6 +2082,18 @@ public:
>     return NULL;
>   }
> 
> +  /// This callback is used to prepare for a volatile or atomic load.
> +  /// It takes a chain node as input and returns the chain for the load itself.
> +  ///
> +  /// Having a callback like this is necessary for targets like SystemZ,
> +  /// which allows a CPU to reuse the result of a previous load indefinitely,
> +  /// even if a cache-coherent store is performed by another CPU.  The defalut
> +  /// implementation does nothing.
> +  virtual SDValue prepareVolatileOrAtomicLoad(SDValue Chain, SDLoc DL,
> +                                              SelectionDAG &DAG) const {
> +    return Chain;
> +  }
> +
>   /// This callback is invoked by the type legalizer to legalize nodes with an
>   /// illegal operand type but legal result types.  It replaces the
>   /// LowerOperation callback in the type Legalizer.  The reason we can not do
> diff --git a/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp b/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
> index b1d247e..071781a 100644
> --- a/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
> +++ b/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
> @@ -3400,7 +3400,7 @@ void SelectionDAGBuilder::visitLoad(const LoadInst &I) {
> 
>   SDValue Root;
>   bool ConstantMemory = false;
> -  if (I.isVolatile() || NumValues > MaxParallelChains)
> +  if (isVolatile || NumValues > MaxParallelChains)
>     // Serialize volatile loads with other side effects.
>     Root = getRoot();
>   else if (AA->pointsToConstantMemory(
> @@ -3413,6 +3413,10 @@ void SelectionDAGBuilder::visitLoad(const LoadInst &I) {
>     Root = DAG.getRoot();
>   }
> 
> +  const TargetLowering *TLI = TM.getTargetLowering();
> +  if (isVolatile)
> +    Root = TLI->prepareVolatileOrAtomicLoad(Root, getCurSDLoc(), DAG);
> +
>   SmallVector<SDValue, 4> Values(NumValues);
>   SmallVector<SDValue, 4> Chains(std::min(unsigned(MaxParallelChains),
>                                           NumValues));
> @@ -3637,6 +3641,7 @@ void SelectionDAGBuilder::visitAtomicLoad(const LoadInst &I) {
>   if (I.getAlignment() < VT.getSizeInBits() / 8)
>     report_fatal_error("Cannot generate unaligned atomic load");
> 
> +  InChain = TLI->prepareVolatileOrAtomicLoad(InChain, dl, DAG);
>   SDValue L =
>     DAG.getAtomic(ISD::ATOMIC_LOAD, dl, VT, VT, InChain,
>                   getValue(I.getPointerOperand()),
> 
> The rest are all local to the target.
> 
> Thanks,
> Richard
>