[llvm] r224940 - [X86] Fix some cases where some 8-bit instructions were marked as being convertible to three address instructions, but aren't really.
Craig Topper
craig.topper at gmail.com
Mon Jan 5 13:54:12 PST 2015
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.
On Mon, Jan 5, 2015 at 1:43 PM, Quentin Colombet <qcolombet at apple.com>
wrote:
>
> On Jan 5, 2015, at 12:27 PM, Craig Topper <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>
> 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>
>> 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
>> > 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
>> >
>> ==============================================================================
>> > --- 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
>> > 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/dd28acf2/attachment.html>
More information about the llvm-commits
mailing list