Villmow, Micah
2012-Nov-06 21:45 UTC
[LLVMdev] FW: Bug in SelectionDAG visitTargetIntrinsic
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 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. 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. This patch sound good? Thanks, Micah -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20121106/7e359891/attachment.html> -------------- next part -------------- An embedded and charset-unspecified text was scrubbed... Name: target_intrinsic_incorrect_chain.txt URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20121106/7e359891/attachment.txt>
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;
This doesn't seem right. If a call is marked ReadNone, it doesn't seem
legal to select it to an intrinsic that read / write memory. By definition a
"ReadNone" function cannot touch memory, no?
Evan
On Nov 6, 2012, at 1:45 PM, "Villmow, Micah" <Micah.Villmow at
amd.com> 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 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.
>
> 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.
>
> This patch sound good?
>
> Thanks,
> Micah
>
>
<target_intrinsic_incorrect_chain.txt>_______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20121106/6a859705/attachment.html>
At this point, the intrinsic has already been selected, as it was done in its
grandparent function(visitCall). This function is only generating the SDNodes
that corresponds to the intrinsic and the question is should the call be used to
determine whether the chain should be generated, or should the intrinsic be used
instead? Since Tablegen uses the intrinsic to do so, I believe normalizing the
behavior here is the correct approahc.
Micah
From: Evan Cheng [mailto:evan.cheng at apple.com]
Sent: Tuesday, November 06, 2012 3:42 PM
To: Villmow, Micah
Cc: llvmdev at cs.uiuc.edu List
Subject: Re: [LLVMdev] Bug in SelectionDAG visitTargetIntrinsic
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;
This doesn't seem right. If a call is marked ReadNone, it doesn't seem
legal to select it to an intrinsic that read / write memory. By definition a
"ReadNone" function cannot touch memory, no?
Evan
On Nov 6, 2012, at 1:45 PM, "Villmow, Micah" <Micah.Villmow at
amd.com<mailto:Micah.Villmow at amd.com>> wrote:
From: Villmow, Micah
Sent: Tuesday, November 06, 2012 1:37 PM
To: 'llvm-dev at cs.uiuc.edu<mailto: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 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.
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.
This patch sound good?
Thanks,
Micah
<target_intrinsic_incorrect_chain.txt>_______________________________________________
LLVM Developers mailing list
LLVMdev at cs.uiuc.edu<mailto:LLVMdev at cs.uiuc.edu>
http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20121106/5542e748/attachment.html>
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)
Hi Evan, On 07/11/12 00:42, Evan Cheng wrote:> > 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; > > This doesn't seem right. If a call is marked ReadNone, it doesn't seem legal to > select it to an intrinsic that read / write memory. By definition a "ReadNone" > function cannot touch memory, no?you can have a readnone call to a function that writes memory, it's just that you somehow know that this particular call doesn't write memory. For example consider a call to int @my_func(int param, bool write_mem); which only writes memory if write_mem is 'true'. Then it would be correct to mark calls to my_func "readonly" when the write_mem parameter is 'false', even though the my_func function itself could't have the readonly parameter, since it can write memory sometimes, i.e. when write_mem is 'true'. Presumably Micah has an intrinsic that works this way: it is capable of writing memory, but in some special identifiable circumstances it doesn't. Ciao, Duncan.> > Evan > > On Nov 6, 2012, at 1:45 PM, "Villmow, Micah" <Micah.Villmow at amd.com > <mailto:Micah.Villmow at amd.com>> wrote: > >> *From:*Villmow, Micah >> *Sent:*Tuesday, November 06, 2012 1:37 PM >> *To:*'llvm-dev at cs.uiuc.edu <mailto: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 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. >> 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. >> This patch sound good? >> Thanks, >> Micah >> <target_intrinsic_incorrect_chain.txt>_______________________________________________ >> LLVM Developers mailing list >> LLVMdev at cs.uiuc.edu <mailto:LLVMdev at cs.uiuc.edu>http://llvm.cs.uiuc.edu >> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev > > > > _______________________________________________ > LLVM Developers mailing list > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev >