<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><br class=""><div><blockquote type="cite" class=""><div class="">On Jan 5, 2015, at 1:54 PM, Craig Topper <<a href="mailto:craig.topper@gmail.com" class="">craig.topper@gmail.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div dir="ltr" class="">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.</div></div></blockquote><div><br class=""></div><div>You mean in the X86 specific code?</div><div>If yes, I agree and we could even have a llvm_unreachable.</div><div><br class=""></div>Thanks,</div><div>-Quentin</div><div><br class=""><blockquote type="cite" class=""><div class=""><div class="gmail_extra"><br class=""><div class="gmail_quote">On Mon, Jan 5, 2015 at 1:43 PM, Quentin Colombet <span dir="ltr" class=""><<a href="mailto:qcolombet@apple.com" target="_blank" class="">qcolombet@apple.com</a>></span> wrote:<br class=""><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word" class=""><br class=""><div class=""><span class=""><blockquote type="cite" class=""><div class="">On Jan 5, 2015, at 12:27 PM, Craig Topper <<a href="mailto:craig.topper@gmail.com" target="_blank" class="">craig.topper@gmail.com</a>> wrote:</div><br class=""><div class=""><div dir="ltr" class="">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. </div></div></blockquote><div class=""><br class=""></div></span><div class="">I see, thanks for the clarification.</div><span class=""><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class="">Perhaps we should have an assertion in convertToThreeAddress in the default case of the switch to catch more of these mistagged instructions.</div></div></blockquote><div class=""><br class=""></div></span><div class="">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.</div><div class="">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.</div><div class="">What do you think?</div><div class=""><br class=""></div><div class="">Thanks,</div><div class=""><div class="h5"><div class="">Q.</div><br class=""><blockquote type="cite" class=""><div class=""><div class="gmail_extra"><br class=""><div class="gmail_quote">On Mon, Jan 5, 2015 at 12:18 PM, Quentin Colombet <span dir="ltr" class=""><<a href="mailto:qcolombet@apple.com" target="_blank" class="">qcolombet@apple.com</a>></span> wrote:<br class=""><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Craig,<br class="">
<br class="">
Would it be possible to have a test case for this?<br class="">
<br class="">
Thanks,<br class="">
-Quentin<br class="">
<br class="">
> On Dec 29, 2014, at 8:25 AM, Craig Topper <<a href="mailto:craig.topper@gmail.com" target="_blank" class="">craig.topper@gmail.com</a>> wrote:<br class="">
><br class="">
> Author: ctopper<br class="">
> Date: Mon Dec 29 10:25:26 2014<br class="">
> New Revision: 224940<br class="">
><br class="">
> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=224940&view=rev" target="_blank" class="">http://llvm.org/viewvc/llvm-project?rev=224940&view=rev</a><br class="">
> Log:<br class="">
> [X86] Fix some cases where some 8-bit instructions were marked as being convertible to three address instructions, but aren't really.<br class="">
><br class="">
> Modified:<br class="">
> llvm/trunk/lib/Target/X86/X86InstrArithmetic.td<br class="">
><br class="">
> Modified: llvm/trunk/lib/Target/X86/X86InstrArithmetic.td<br class="">
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/X86/X86InstrArithmetic.td?rev=224940&r1=224939&r2=224940&view=diff" target="_blank" class="">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/X86/X86InstrArithmetic.td?rev=224940&r1=224939&r2=224940&view=diff</a><br class="">
> ==============================================================================<br class="">
> --- llvm/trunk/lib/Target/X86/X86InstrArithmetic.td (original)<br class="">
> +++ llvm/trunk/lib/Target/X86/X86InstrArithmetic.td Mon Dec 29 10:25:26 2014<br class="">
> @@ -999,12 +999,13 @@ multiclass ArithBinOp_RF<bits<8> BaseOpc<br class="">
> bit CommutableRR, bit ConvertibleToThreeAddress> {<br class="">
> let Defs = [EFLAGS] in {<br class="">
> let Constraints = "$src1 = $dst" in {<br class="">
> - let isCommutable = CommutableRR,<br class="">
> - isConvertibleToThreeAddress = ConvertibleToThreeAddress in {<br class="">
> + let isCommutable = CommutableRR in {<br class="">
> def NAME#8rr : BinOpRR_RF<BaseOpc, mnemonic, Xi8 , opnodeflag>;<br class="">
> - def NAME#16rr : BinOpRR_RF<BaseOpc, mnemonic, Xi16, opnodeflag>;<br class="">
> - def NAME#32rr : BinOpRR_RF<BaseOpc, mnemonic, Xi32, opnodeflag>;<br class="">
> - def NAME#64rr : BinOpRR_RF<BaseOpc, mnemonic, Xi64, opnodeflag>;<br class="">
> + let isConvertibleToThreeAddress = ConvertibleToThreeAddress in {<br class="">
> + def NAME#16rr : BinOpRR_RF<BaseOpc, mnemonic, Xi16, opnodeflag>;<br class="">
> + def NAME#32rr : BinOpRR_RF<BaseOpc, mnemonic, Xi32, opnodeflag>;<br class="">
> + def NAME#64rr : BinOpRR_RF<BaseOpc, mnemonic, Xi64, opnodeflag>;<br class="">
> + } // isConvertibleToThreeAddress<br class="">
> } // isCommutable<br class="">
><br class="">
> def NAME#8rr_REV : BinOpRR_Rev<BaseOpc2, mnemonic, Xi8>;<br class="">
> @@ -1017,6 +1018,8 @@ multiclass ArithBinOp_RF<bits<8> BaseOpc<br class="">
> def NAME#32rm : BinOpRM_RF<BaseOpc2, mnemonic, Xi32, opnodeflag>;<br class="">
> def NAME#64rm : BinOpRM_RF<BaseOpc2, mnemonic, Xi64, opnodeflag>;<br class="">
><br class="">
> + def NAME#8ri : BinOpRI_RF<0x80, mnemonic, Xi8 , opnodeflag, RegMRM>;<br class="">
> +<br class="">
> let isConvertibleToThreeAddress = ConvertibleToThreeAddress in {<br class="">
> // NOTE: These are order specific, we want the ri8 forms to be listed<br class="">
> // first so that they are slightly preferred to the ri forms.<br class="">
> @@ -1024,7 +1027,6 @@ multiclass ArithBinOp_RF<bits<8> BaseOpc<br class="">
> def NAME#32ri8 : BinOpRI8_RF<0x82, mnemonic, Xi32, opnodeflag, RegMRM>;<br class="">
> def NAME#64ri8 : BinOpRI8_RF<0x82, mnemonic, Xi64, opnodeflag, RegMRM>;<br class="">
><br class="">
> - def NAME#8ri : BinOpRI_RF<0x80, mnemonic, Xi8 , opnodeflag, RegMRM>;<br class="">
> def NAME#16ri : BinOpRI_RF<0x80, mnemonic, Xi16, opnodeflag, RegMRM>;<br class="">
> def NAME#32ri : BinOpRI_RF<0x80, mnemonic, Xi32, opnodeflag, RegMRM>;<br class="">
> def NAME#64ri32: BinOpRI_RF<0x80, mnemonic, Xi64, opnodeflag, RegMRM>;<br class="">
> @@ -1080,12 +1082,13 @@ multiclass ArithBinOp_RFF<bits<8> BaseOp<br class="">
> bit ConvertibleToThreeAddress> {<br class="">
> let Uses = [EFLAGS], Defs = [EFLAGS] in {<br class="">
> let Constraints = "$src1 = $dst" in {<br class="">
> - let isCommutable = CommutableRR,<br class="">
> - isConvertibleToThreeAddress = ConvertibleToThreeAddress in {<br class="">
> + let isCommutable = CommutableRR in {<br class="">
> def NAME#8rr : BinOpRR_RFF<BaseOpc, mnemonic, Xi8 , opnode>;<br class="">
> - def NAME#16rr : BinOpRR_RFF<BaseOpc, mnemonic, Xi16, opnode>;<br class="">
> - def NAME#32rr : BinOpRR_RFF<BaseOpc, mnemonic, Xi32, opnode>;<br class="">
> - def NAME#64rr : BinOpRR_RFF<BaseOpc, mnemonic, Xi64, opnode>;<br class="">
> + let isConvertibleToThreeAddress = ConvertibleToThreeAddress in {<br class="">
> + def NAME#16rr : BinOpRR_RFF<BaseOpc, mnemonic, Xi16, opnode>;<br class="">
> + def NAME#32rr : BinOpRR_RFF<BaseOpc, mnemonic, Xi32, opnode>;<br class="">
> + def NAME#64rr : BinOpRR_RFF<BaseOpc, mnemonic, Xi64, opnode>;<br class="">
> + } // isConvertibleToThreeAddress<br class="">
> } // isCommutable<br class="">
><br class="">
> def NAME#8rr_REV : BinOpRR_RFF_Rev<BaseOpc2, mnemonic, Xi8>;<br class="">
> @@ -1098,6 +1101,8 @@ multiclass ArithBinOp_RFF<bits<8> BaseOp<br class="">
> def NAME#32rm : BinOpRM_RFF<BaseOpc2, mnemonic, Xi32, opnode>;<br class="">
> def NAME#64rm : BinOpRM_RFF<BaseOpc2, mnemonic, Xi64, opnode>;<br class="">
><br class="">
> + def NAME#8ri : BinOpRI_RFF<0x80, mnemonic, Xi8 , opnode, RegMRM>;<br class="">
> +<br class="">
> let isConvertibleToThreeAddress = ConvertibleToThreeAddress in {<br class="">
> // NOTE: These are order specific, we want the ri8 forms to be listed<br class="">
> // first so that they are slightly preferred to the ri forms.<br class="">
> @@ -1105,7 +1110,6 @@ multiclass ArithBinOp_RFF<bits<8> BaseOp<br class="">
> def NAME#32ri8 : BinOpRI8_RFF<0x82, mnemonic, Xi32, opnode, RegMRM>;<br class="">
> def NAME#64ri8 : BinOpRI8_RFF<0x82, mnemonic, Xi64, opnode, RegMRM>;<br class="">
><br class="">
> - def NAME#8ri : BinOpRI_RFF<0x80, mnemonic, Xi8 , opnode, RegMRM>;<br class="">
> def NAME#16ri : BinOpRI_RFF<0x80, mnemonic, Xi16, opnode, RegMRM>;<br class="">
> def NAME#32ri : BinOpRI_RFF<0x80, mnemonic, Xi32, opnode, RegMRM>;<br class="">
> def NAME#64ri32: BinOpRI_RFF<0x80, mnemonic, Xi64, opnode, RegMRM>;<br class="">
> @@ -1158,12 +1162,13 @@ multiclass ArithBinOp_F<bits<8> BaseOpc,<br class="">
> SDNode opnode,<br class="">
> bit CommutableRR, bit ConvertibleToThreeAddress> {<br class="">
> let Defs = [EFLAGS] in {<br class="">
> - let isCommutable = CommutableRR,<br class="">
> - isConvertibleToThreeAddress = ConvertibleToThreeAddress in {<br class="">
> + let isCommutable = CommutableRR in {<br class="">
> def NAME#8rr : BinOpRR_F<BaseOpc, mnemonic, Xi8 , opnode>;<br class="">
> - def NAME#16rr : BinOpRR_F<BaseOpc, mnemonic, Xi16, opnode>;<br class="">
> - def NAME#32rr : BinOpRR_F<BaseOpc, mnemonic, Xi32, opnode>;<br class="">
> - def NAME#64rr : BinOpRR_F<BaseOpc, mnemonic, Xi64, opnode>;<br class="">
> + let isConvertibleToThreeAddress = ConvertibleToThreeAddress in {<br class="">
> + def NAME#16rr : BinOpRR_F<BaseOpc, mnemonic, Xi16, opnode>;<br class="">
> + def NAME#32rr : BinOpRR_F<BaseOpc, mnemonic, Xi32, opnode>;<br class="">
> + def NAME#64rr : BinOpRR_F<BaseOpc, mnemonic, Xi64, opnode>;<br class="">
> + }<br class="">
> } // isCommutable<br class="">
><br class="">
> def NAME#8rr_REV : BinOpRR_F_Rev<BaseOpc2, mnemonic, Xi8>;<br class="">
> @@ -1176,6 +1181,8 @@ multiclass ArithBinOp_F<bits<8> BaseOpc,<br class="">
> def NAME#32rm : BinOpRM_F<BaseOpc2, mnemonic, Xi32, opnode>;<br class="">
> def NAME#64rm : BinOpRM_F<BaseOpc2, mnemonic, Xi64, opnode>;<br class="">
><br class="">
> + def NAME#8ri : BinOpRI_F<0x80, mnemonic, Xi8 , opnode, RegMRM>;<br class="">
> +<br class="">
> let isConvertibleToThreeAddress = ConvertibleToThreeAddress in {<br class="">
> // NOTE: These are order specific, we want the ri8 forms to be listed<br class="">
> // first so that they are slightly preferred to the ri forms.<br class="">
> @@ -1183,7 +1190,6 @@ multiclass ArithBinOp_F<bits<8> BaseOpc,<br class="">
> def NAME#32ri8 : BinOpRI8_F<0x82, mnemonic, Xi32, opnode, RegMRM>;<br class="">
> def NAME#64ri8 : BinOpRI8_F<0x82, mnemonic, Xi64, opnode, RegMRM>;<br class="">
><br class="">
> - def NAME#8ri : BinOpRI_F<0x80, mnemonic, Xi8 , opnode, RegMRM>;<br class="">
> def NAME#16ri : BinOpRI_F<0x80, mnemonic, Xi16, opnode, RegMRM>;<br class="">
> def NAME#32ri : BinOpRI_F<0x80, mnemonic, Xi32, opnode, RegMRM>;<br class="">
> def NAME#64ri32: BinOpRI_F<0x80, mnemonic, Xi64, opnode, RegMRM>;<br class="">
><br class="">
><br class="">
> _______________________________________________<br class="">
> llvm-commits mailing list<br class="">
> <a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank" class="">llvm-commits@cs.uiuc.edu</a><br class="">
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank" class="">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br class="">
<br class="">
</blockquote></div><br class=""><br clear="all" class=""><div class=""><br class=""></div>-- <br class=""><div class="">~Craig</div>
</div>
</div></blockquote></div></div></div><br class=""></div></blockquote></div><br class=""><br clear="all" class=""><div class=""><br class=""></div>-- <br class=""><div class="gmail_signature">~Craig</div>
</div>
</div></blockquote></div><br class=""></body></html>