[llvm] r224940 - [X86] Fix some cases where some 8-bit instructions were marked as being convertible to three address instructions, but aren't really.
Quentin Colombet
qcolombet at apple.com
Mon Jan 5 16:07:33 PST 2015
> On Jan 5, 2015, at 1:54 PM, Craig Topper <craig.topper at gmail.com> wrote:
>
> The routine returns nullptr if it doesn't make a change. The caller checks for that. The default case in the switch always returns nullptr. Other instrucitons conditionally return nullptr if they can't make a change. But I don't think the default in the switch should ever execute if the tablegen files only mark instructions that have an implementation in the switch. So we should be able to assert the default case assuming tablegen is correct.
You mean in the X86 specific code?
If yes, I agree and we could even have a llvm_unreachable.
Thanks,
-Quentin
>
> On Mon, Jan 5, 2015 at 1:43 PM, Quentin Colombet <qcolombet at apple.com <mailto:qcolombet at apple.com>> wrote:
>
>> On Jan 5, 2015, at 12:27 PM, Craig Topper <craig.topper at gmail.com <mailto:craig.topper at gmail.com>> wrote:
>>
>> No because they just end up getting ignored in the convertToThreeAddress function in X86InstrInfo.cpp. This change just stops the TwoAddressInstructionPass from even trying to call convertoToThreeAddress for these instructions.
>
> I see, thanks for the clarification.
>
>> Perhaps we should have an assertion in convertToThreeAddress in the default case of the switch to catch more of these mistagged instructions.
>
> I have not checked what does the documentation say for the related target hook, but I guess it is valid to return an “invalid” opcode. Think instruction that is convertible to three address but only for some inputs.
> The bottom line is, I do not think the assertion will be appropriate, but we may consider adding a debug output to express that the conversion did not happen (if that is not already here) and perhaps some internal option to turn that into a crash for debugging purposes.
> What do you think?
>
> Thanks,
> Q.
>
>>
>> On Mon, Jan 5, 2015 at 12:18 PM, Quentin Colombet <qcolombet at apple.com <mailto:qcolombet at apple.com>> wrote:
>> Hi Craig,
>>
>> Would it be possible to have a test case for this?
>>
>> Thanks,
>> -Quentin
>>
>> > On Dec 29, 2014, at 8:25 AM, Craig Topper <craig.topper at gmail.com <mailto:craig.topper at gmail.com>> wrote:
>> >
>> > Author: ctopper
>> > Date: Mon Dec 29 10:25:26 2014
>> > New Revision: 224940
>> >
>> > URL: http://llvm.org/viewvc/llvm-project?rev=224940&view=rev <http://llvm.org/viewvc/llvm-project?rev=224940&view=rev>
>> > Log:
>> > [X86] Fix some cases where some 8-bit instructions were marked as being convertible to three address instructions, but aren't really.
>> >
>> > Modified:
>> > llvm/trunk/lib/Target/X86/X86InstrArithmetic.td
>> >
>> > Modified: llvm/trunk/lib/Target/X86/X86InstrArithmetic.td
>> > URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/X86/X86InstrArithmetic.td?rev=224940&r1=224939&r2=224940&view=diff <http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/X86/X86InstrArithmetic.td?rev=224940&r1=224939&r2=224940&view=diff>
>> > ==============================================================================
>> > --- llvm/trunk/lib/Target/X86/X86InstrArithmetic.td (original)
>> > +++ llvm/trunk/lib/Target/X86/X86InstrArithmetic.td Mon Dec 29 10:25:26 2014
>> > @@ -999,12 +999,13 @@ multiclass ArithBinOp_RF<bits<8> BaseOpc
>> > bit CommutableRR, bit ConvertibleToThreeAddress> {
>> > let Defs = [EFLAGS] in {
>> > let Constraints = "$src1 = $dst" in {
>> > - let isCommutable = CommutableRR,
>> > - isConvertibleToThreeAddress = ConvertibleToThreeAddress in {
>> > + let isCommutable = CommutableRR in {
>> > def NAME#8rr : BinOpRR_RF<BaseOpc, mnemonic, Xi8 , opnodeflag>;
>> > - def NAME#16rr : BinOpRR_RF<BaseOpc, mnemonic, Xi16, opnodeflag>;
>> > - def NAME#32rr : BinOpRR_RF<BaseOpc, mnemonic, Xi32, opnodeflag>;
>> > - def NAME#64rr : BinOpRR_RF<BaseOpc, mnemonic, Xi64, opnodeflag>;
>> > + let isConvertibleToThreeAddress = ConvertibleToThreeAddress in {
>> > + def NAME#16rr : BinOpRR_RF<BaseOpc, mnemonic, Xi16, opnodeflag>;
>> > + def NAME#32rr : BinOpRR_RF<BaseOpc, mnemonic, Xi32, opnodeflag>;
>> > + def NAME#64rr : BinOpRR_RF<BaseOpc, mnemonic, Xi64, opnodeflag>;
>> > + } // isConvertibleToThreeAddress
>> > } // isCommutable
>> >
>> > def NAME#8rr_REV : BinOpRR_Rev<BaseOpc2, mnemonic, Xi8>;
>> > @@ -1017,6 +1018,8 @@ multiclass ArithBinOp_RF<bits<8> BaseOpc
>> > def NAME#32rm : BinOpRM_RF<BaseOpc2, mnemonic, Xi32, opnodeflag>;
>> > def NAME#64rm : BinOpRM_RF<BaseOpc2, mnemonic, Xi64, opnodeflag>;
>> >
>> > + def NAME#8ri : BinOpRI_RF<0x80, mnemonic, Xi8 , opnodeflag, RegMRM>;
>> > +
>> > let isConvertibleToThreeAddress = ConvertibleToThreeAddress in {
>> > // NOTE: These are order specific, we want the ri8 forms to be listed
>> > // first so that they are slightly preferred to the ri forms.
>> > @@ -1024,7 +1027,6 @@ multiclass ArithBinOp_RF<bits<8> BaseOpc
>> > def NAME#32ri8 : BinOpRI8_RF<0x82, mnemonic, Xi32, opnodeflag, RegMRM>;
>> > def NAME#64ri8 : BinOpRI8_RF<0x82, mnemonic, Xi64, opnodeflag, RegMRM>;
>> >
>> > - def NAME#8ri : BinOpRI_RF<0x80, mnemonic, Xi8 , opnodeflag, RegMRM>;
>> > def NAME#16ri : BinOpRI_RF<0x80, mnemonic, Xi16, opnodeflag, RegMRM>;
>> > def NAME#32ri : BinOpRI_RF<0x80, mnemonic, Xi32, opnodeflag, RegMRM>;
>> > def NAME#64ri32: BinOpRI_RF<0x80, mnemonic, Xi64, opnodeflag, RegMRM>;
>> > @@ -1080,12 +1082,13 @@ multiclass ArithBinOp_RFF<bits<8> BaseOp
>> > bit ConvertibleToThreeAddress> {
>> > let Uses = [EFLAGS], Defs = [EFLAGS] in {
>> > let Constraints = "$src1 = $dst" in {
>> > - let isCommutable = CommutableRR,
>> > - isConvertibleToThreeAddress = ConvertibleToThreeAddress in {
>> > + let isCommutable = CommutableRR in {
>> > def NAME#8rr : BinOpRR_RFF<BaseOpc, mnemonic, Xi8 , opnode>;
>> > - def NAME#16rr : BinOpRR_RFF<BaseOpc, mnemonic, Xi16, opnode>;
>> > - def NAME#32rr : BinOpRR_RFF<BaseOpc, mnemonic, Xi32, opnode>;
>> > - def NAME#64rr : BinOpRR_RFF<BaseOpc, mnemonic, Xi64, opnode>;
>> > + let isConvertibleToThreeAddress = ConvertibleToThreeAddress in {
>> > + def NAME#16rr : BinOpRR_RFF<BaseOpc, mnemonic, Xi16, opnode>;
>> > + def NAME#32rr : BinOpRR_RFF<BaseOpc, mnemonic, Xi32, opnode>;
>> > + def NAME#64rr : BinOpRR_RFF<BaseOpc, mnemonic, Xi64, opnode>;
>> > + } // isConvertibleToThreeAddress
>> > } // isCommutable
>> >
>> > def NAME#8rr_REV : BinOpRR_RFF_Rev<BaseOpc2, mnemonic, Xi8>;
>> > @@ -1098,6 +1101,8 @@ multiclass ArithBinOp_RFF<bits<8> BaseOp
>> > def NAME#32rm : BinOpRM_RFF<BaseOpc2, mnemonic, Xi32, opnode>;
>> > def NAME#64rm : BinOpRM_RFF<BaseOpc2, mnemonic, Xi64, opnode>;
>> >
>> > + def NAME#8ri : BinOpRI_RFF<0x80, mnemonic, Xi8 , opnode, RegMRM>;
>> > +
>> > let isConvertibleToThreeAddress = ConvertibleToThreeAddress in {
>> > // NOTE: These are order specific, we want the ri8 forms to be listed
>> > // first so that they are slightly preferred to the ri forms.
>> > @@ -1105,7 +1110,6 @@ multiclass ArithBinOp_RFF<bits<8> BaseOp
>> > def NAME#32ri8 : BinOpRI8_RFF<0x82, mnemonic, Xi32, opnode, RegMRM>;
>> > def NAME#64ri8 : BinOpRI8_RFF<0x82, mnemonic, Xi64, opnode, RegMRM>;
>> >
>> > - def NAME#8ri : BinOpRI_RFF<0x80, mnemonic, Xi8 , opnode, RegMRM>;
>> > def NAME#16ri : BinOpRI_RFF<0x80, mnemonic, Xi16, opnode, RegMRM>;
>> > def NAME#32ri : BinOpRI_RFF<0x80, mnemonic, Xi32, opnode, RegMRM>;
>> > def NAME#64ri32: BinOpRI_RFF<0x80, mnemonic, Xi64, opnode, RegMRM>;
>> > @@ -1158,12 +1162,13 @@ multiclass ArithBinOp_F<bits<8> BaseOpc,
>> > SDNode opnode,
>> > bit CommutableRR, bit ConvertibleToThreeAddress> {
>> > let Defs = [EFLAGS] in {
>> > - let isCommutable = CommutableRR,
>> > - isConvertibleToThreeAddress = ConvertibleToThreeAddress in {
>> > + let isCommutable = CommutableRR in {
>> > def NAME#8rr : BinOpRR_F<BaseOpc, mnemonic, Xi8 , opnode>;
>> > - def NAME#16rr : BinOpRR_F<BaseOpc, mnemonic, Xi16, opnode>;
>> > - def NAME#32rr : BinOpRR_F<BaseOpc, mnemonic, Xi32, opnode>;
>> > - def NAME#64rr : BinOpRR_F<BaseOpc, mnemonic, Xi64, opnode>;
>> > + let isConvertibleToThreeAddress = ConvertibleToThreeAddress in {
>> > + def NAME#16rr : BinOpRR_F<BaseOpc, mnemonic, Xi16, opnode>;
>> > + def NAME#32rr : BinOpRR_F<BaseOpc, mnemonic, Xi32, opnode>;
>> > + def NAME#64rr : BinOpRR_F<BaseOpc, mnemonic, Xi64, opnode>;
>> > + }
>> > } // isCommutable
>> >
>> > def NAME#8rr_REV : BinOpRR_F_Rev<BaseOpc2, mnemonic, Xi8>;
>> > @@ -1176,6 +1181,8 @@ multiclass ArithBinOp_F<bits<8> BaseOpc,
>> > def NAME#32rm : BinOpRM_F<BaseOpc2, mnemonic, Xi32, opnode>;
>> > def NAME#64rm : BinOpRM_F<BaseOpc2, mnemonic, Xi64, opnode>;
>> >
>> > + def NAME#8ri : BinOpRI_F<0x80, mnemonic, Xi8 , opnode, RegMRM>;
>> > +
>> > let isConvertibleToThreeAddress = ConvertibleToThreeAddress in {
>> > // NOTE: These are order specific, we want the ri8 forms to be listed
>> > // first so that they are slightly preferred to the ri forms.
>> > @@ -1183,7 +1190,6 @@ multiclass ArithBinOp_F<bits<8> BaseOpc,
>> > def NAME#32ri8 : BinOpRI8_F<0x82, mnemonic, Xi32, opnode, RegMRM>;
>> > def NAME#64ri8 : BinOpRI8_F<0x82, mnemonic, Xi64, opnode, RegMRM>;
>> >
>> > - def NAME#8ri : BinOpRI_F<0x80, mnemonic, Xi8 , opnode, RegMRM>;
>> > def NAME#16ri : BinOpRI_F<0x80, mnemonic, Xi16, opnode, RegMRM>;
>> > def NAME#32ri : BinOpRI_F<0x80, mnemonic, Xi32, opnode, RegMRM>;
>> > def NAME#64ri32: BinOpRI_F<0x80, mnemonic, Xi64, opnode, RegMRM>;
>> >
>> >
>> > _______________________________________________
>> > llvm-commits mailing list
>> > llvm-commits at cs.uiuc.edu <mailto:llvm-commits at cs.uiuc.edu>
>> > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits <http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits>
>>
>>
>>
>>
>> --
>> ~Craig
>
>
>
>
> --
> ~Craig
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150105/2a461895/attachment.html>
More information about the llvm-commits
mailing list