[LLVMdev] FW: Bug in SelectionDAG visitTargetIntrinsic

[Duncan Sands]

Hi Micah,

On 06/11/12 22:45, Villmow, Micah wrote:
> *From:*Villmow, Micah
> *Sent:* Tuesday, November 06, 2012 1:37 PM
> *To:* 'llvm-dev at cs.uiuc.edu'
> *Cc:* Guo, Xiaoyi
> *Subject:* Bug in SelectionDAG visitTargetIntrinsic
>
> We ran into a problem where specifying IntrNoMem was causing our instruction
> selection to fail with target specific intrinsics. After looking into the code
> and ISel debug it looks like tablegen and SelectionDAG are using different
> criteria to generate code for intrinsic_w_chain vs intrinsic_wo_chain.
>
> In CodeGenDAGPatterns.cpp, tablegen decides based on whether IntrNoMem is set or
> not. However with SelectionDAG, whether to use a chain or not is determined by
> the call site attributes and not by the intrinsic.
>
> So, we can get the situation where the call site has a different attribute than
> the intrinsic, and this causes selection dag to fail.

I'm curious to know how this occurs?  I mean, what code is generating a call to
an intrinsic with different attributes on the call site?

> I believe that this is wrong and that whether a chain should be generated or not
> should come from only the intrinsic and not the call site. Since the mapping of
> call -> intrinsic is by function name only, it should not matter if the readnone
> attribute is set or not as that is irrelevant to the code generator. Only what
> is set in the tablegen definition should be determine how the intrinsic is
> generated.

In my opinion whether the node returns a chain result or not should only depend
on the intrinsic, not on the call site.  However if the intrinsic has a chain
but the call-site says "read-only" you could, as an optimization, not use the
chain, so getting some benefit from the "read-only" at the codegen level.

> So, I'm proposing the following patch. What this patch does is instead of
> relying on the call site to determine if a chain is required, use instead the
> read/write attributes of the intrinsic from the backend instead. There is not
> much documentation on target intrinsics and no other backend uses them in this
> manner.

As for your patch, you need a testcase.  Also,

> @@ -3508,9 +3508,12 @@
>  /// visitTargetIntrinsic - Lower a call of a target intrinsic to an INTRINSIC
>  /// node.
>  void SelectionDAGBuilder::visitTargetIntrinsic(const CallInst &I,
> -                                               unsigned Intrinsic) {
> -  bool HasChain = !I.doesNotAccessMemory();
> -  bool OnlyLoad = HasChain && I.onlyReadsMemory();
> +                                             unsigned Intrinsic) {
> +  // Info is set by getTgtMemInstrinsic
> +  TargetLowering::IntrinsicInfo Info;
> +  bool IsTgtIntrinsic = TLI.getTgtMemIntrinsic(Info, I, Intrinsic);
> +  bool HasChain = Info.readMem || Info.writeMem;
> +  bool OnlyLoad = HasChain && Info.readMem;

I don't see what this issue has to do with target intrinsics.  Can't you write
some IR by hand that calls some ordinary intrinsic with an unexpected "readnone"
flag and hit the same issue?

Ciao, Duncan.

>
>    // Build the operand list.
>    SmallVector<SDValue, 8> Ops;
> @@ -3523,10 +3526,6 @@
>      }
>    }
>
> -  // Info is set by getTgtMemInstrinsic
> -  TargetLowering::IntrinsicInfo Info;
> -  bool IsTgtIntrinsic = TLI.getTgtMemIntrinsic(Info, I, Intrinsic);
> -
>    // Add the intrinsic ID as an integer operand if it's not a target intrinsic.
>    if (!IsTgtIntrinsic || Info.opc == ISD::INTRINSIC_VOID ||
>        Info.opc == ISD::INTRINSIC_W_CHAIN)