<html><head><meta http-equiv="Content-Type" content="text/html charset=iso-8859-1"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><br><div><div>On May 22, 2013, at 12:33 AM, Duncan Sands <<a href="mailto:duncan.sands@gmail.com">duncan.sands@gmail.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div style="letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;">Hi Michael,<br><br>On 21/05/13 23:53, Michael Gottesman wrote:<br><blockquote type="cite">Hey Duncan. I am going to respond in a backwards order since I think it will<br>yield additional clarity [if it does not, please tell me and I will elaborate = )]:<br><br><blockquote type="cite">Actually, isn't ADDC really exactly the same as UADDO? If so, how about<br>eliminating the ADDC in favour of UADDO? In any case, there's already<br>promotion logic for UADDO, DAGTypeLegalizer::PromoteIntRes_UADDSUBO, so<br>maybe you could take a look.<br></blockquote></blockquote><br>actually I should probably have said: eliminate UADDO, replacing it with ADDC<br>everywhere.<br><br></div></blockquote><div><br></div><div>This seems out of scope for this patch since this patch is a small feature addition patch. Changing UADDO -> ADDC everywhere will definitely require changing a bunch of code in the X86 backend at least if not more backends. If we want to do this, we should file a PR (just due to the magnitude of the work).</div><br><blockquote type="cite"><div style="letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><blockquote type="cite"><br>While ADDC/UADDO seem similar they are meant to model two different things.<br><br>UADDO is meant to model the situation where one performs an addition and wishes<br>to detect overflow as a boolean value and perform some non-arithmetic operation,<br></blockquote><br>How do you know it will be used for non-arithmetic operations?<br></div></blockquote><div><br></div><div>I was basing my assertion on the prior art in the X86 backend.</div><br><blockquote type="cite"><div style="letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><br><blockquote type="cite">not perform additional arithmetic with said overflow value. Thus it makes sense<br>when we are promoting UADDO into an add + setne operation since UADDO is lowered<br>into similar code anyways (see LowerXALUO X86ISelLowering.cpp) meaning no<br>performance hit.<br><br>On the other hand ADDC is meant to model the situation where one wishes to<br>perform additional arithmetic with the overflow value (specifically in a<br>multiprecision arithmetic context), i.e. on intel:<br><br>addq (%rbx), %rax<br>adcq 8(%rbx), %rcx<br>adcq 16(%rbx), %ddx<br>...<br><br>If we were to lower ADDC like UADDO, we would instead have something along the<br>lines of the following non-optimal codegen:<br><br>xorq %rax, %rax<br>addq (%rbx), %rcx<br>setne %al<br>movq %rax, %r8<br>addq 8(%rbx), %r9<br>getne %al<br>addq %r9, %r8<br>...<br><br>Thus the necessity of this distinction. X86 definitely follows said distinction<br>if you look at the lowering.<br></blockquote><br>I disagree. The ADDC and UADDO SDAG nodes have exactly the same definition.<br>IMO, the fact that UADDO codegen is poor is a codegen weakness that should be<br>improved, not a reason to keep two nodes, unless there really is no other way.<br></div></blockquote><div><br></div><div>Again, I feel that changing UADDO -> ADDC to be out of scope for this patch since the patch is really only about handling the lowering of ADDC, not changing a bunch of code to change all usages of UADDO -> ADDC everywhere.</div><br><blockquote type="cite"><div style="letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><br><blockquote type="cite"><br>Which leads me to your first paragraph.<br><br><blockquote type="cite">thanks for adding the ResNo == 0 case, but I don't think it is correct.<br>That's because you continue to return an ADDC code, yet because you zero<br>extended the add operands there is no reason why the extended operands<br>should overflow (set the carry flag) exactly when the original operation<br>would.<br></blockquote><br>This is not completely true. It depends on how you do it. Specifically it<br>depends on how one legalizes the source operations of the addc.<br></blockquote><br>No it doesn't, this is not how type legalization works. Upper bits of promoted<br>integers contain *undefined bits*. If they happen to be zext'd then you just<br>got lucky, you can't rely on it. That's why if you *require* the extra bits to<br>be zero-extended you need to use ZExtPromotedInteger, which explicitly zeros out<br>the extra bits (the optimizers are often able to get rid of the explicit zeroing<br>out if instead they can use an incoming node for which they can prove that the<br>bits are zero already).<br><br>Anyway, this is something of a digression, since promotion *doesn't change the<br>meaning of what you are calculating*. Consider the following operation:<br><br> (i8 x, i1 flag) = addc i8 255, 2<br><br>Then you get:<br><br> x = i8 1<br> flag = i1 1<br><br>On a platform where i8 and i1 have to be promoted to 32 bits, this *must* get<br>legalized to:<br><br> x = a 32 bit integer, where the 8 lower bits are 00000001 (aka i8 1), and the<br>rest can be anything (it doesn't matter what they are, you can choose something<br>convenient)<br><br> flag = a 32 bit integer, where the lowest bit is 1, and the rest can be<br>anything. (If promoting according to the so-called "boolean contents" then<br>the extra bits may have additional requirements, such as being zero- or sign-<br>extended from the lowest bit).<br><br>Now consider what your logic does, supposing first you have to promote ResNo 0.<br>Your logic forms<br><br> (i32 x', i1 flag') = addc i32 255, 2<br><br>So you get:<br><br> x' = i32 257<br> flag' = i1 0 (as this i32 operation does not set the carry flag)<br><br>It is OK to use x' for the promoted value of x, since the 8 lowest bits are<br>00000001. But it is not OK to use flag' for flag, since the lowest bit is<br>zero, not 1.<br><br>In short, the problem is correctly calculating the flag (you have to check if<br>bit 8 of x' was set), not calculating the sum. See the UADDO promotion logic<br>for one way that calculating the flag can be done.<br></div></blockquote><div><br></div><div>Ok. My point was that if we had certain guarantees than it *could* work. On the other hand as you have pointed out in this section of your response we do not have any of those guarantees. The end result is my digression was a waste of text and time = /.</div><div><br></div><div>Would zero extending just ResNo 0 be a satisfactory lowering (i.e. no inputs, just ResNo 0)?</div><div><br></div><div>Thanks for the brain dumping = ).</div><div><br></div><div>Michael</div><br><blockquote type="cite"><div style="letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><br>For instance if<br><blockquote type="cite">one is loading from an 8 bit array, zext to a 32 bit operation and then perform<br>the addc you would have the issue you are thinking of. In contrast, if you could<br>guarantee that the memory access was promoted to 32 bit (since addition of<br>multiprecision operations with different word sizes used yields the same<br>result), one would no longer have that issue. On the other hand, I do not think<br>I can assume this will be done and it is irrelevant to the discussion since we<br>are discussing PromotingIntegerResults = p.<br><br>Given that we are talking about promoting integer results, perhaps the proper<br>way to do this is to just zero extend ResNo == 0 (i.e. not the operands, just<br>the result). What do you think Duncan?<br><br>Michael<br><br>P.S. I realized the reason why I returned SDValue was b/c I thought the integer<br>promotion was like DAGCombine where one returns SDValue if one is not doing<br>anything vs asserting in PromoteIntegerResult where it looks like you assert. = (.<br></blockquote><br>In type legalization, first the target gets to have a go at legalizing the<br>operation. It can return SDValue() if it doesn't know how or doesn't want to.<br>Then the generic logic (eg your ADDC promotion method) kicks in. If it can't<br>legalize the value *then there is no fall-back*: no-one knows how to legalize<br>the value, type legalization has failed. So someone has to assert somewhere!<br>As returning SDValue() is used to mean "I legalized the type by completely<br>zapping the node" there is no convenient way to signal to the machinery that<br>you failed (i.e. having it assert for you), so you have to just assert yourself.</div></blockquote><br><blockquote type="cite"><div style="letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><br>Ciao, Duncan.<br><br><blockquote type="cite"><br>On May 21, 2013, at 12:12 PM, Duncan Sands <<a href="mailto:duncan.sands@gmail.com">duncan.sands@gmail.com</a><br><<a href="mailto:duncan.sands@gmail.com">mailto:duncan.sands@gmail.com</a>>> wrote:<br><br><blockquote type="cite">Hi Michael,<br><br>On 17/05/13 23:33, Michael Gottesman wrote:<br><blockquote type="cite">Hows this look Duncan/Sean?<br></blockquote><br><br>I didn't think about ADDE but at a glance it has the same flaw.<br><br>Ciao, Duncan.<br><br><blockquote type="cite"><br><br><br><br>Michael<br><br>On May 16, 2013, at 10:41 AM, Michael Gottesman <<a href="mailto:mgottesman@apple.com">mgottesman@apple.com</a><br><<a href="mailto:mgottesman@apple.com">mailto:mgottesman@apple.com</a>><br><<a href="mailto:mgottesman@apple.com">mailto:mgottesman@apple.com</a>>> wrote:<br><br><blockquote type="cite">I am trying to be incremental and did not have a need for such lowering. On<br>the other hand if you want I can implement that in this patch. Just state your<br>preference = ).<br><br>Michael<br><br>On May 16, 2013, at 1:34 AM, Duncan Sands <<a href="mailto:duncan.sands@gmail.com">duncan.sands@gmail.com</a><br><<a href="mailto:duncan.sands@gmail.com">mailto:duncan.sands@gmail.com</a>><br><<a href="mailto:duncan.sands@gmail.com">mailto:duncan.sands@gmail.com</a>>> wrote:<br><br><blockquote type="cite">Hi Michael,<br><br>On 15/05/13 19:37, Michael Gottesman wrote:<br><blockquote type="cite">The attached patch teaches selection DAG how to promote the integer carry<br>result of an {add,sub}{c,e} node to a larger integer type. It is a part of a<br>larger set of patches that I will be committing over the next little bit to<br>enable the optimization of uaddo/usubo chains.<br><br>Please Review,<br>Michael<br></blockquote><br>+SDValue DAGTypeLegalizer::PromoteIntRes_ADDSUBC(SDNode *N, unsigned ResNo) {<br>+ if (ResNo == 1)<br>+ return PromoteIntRes_Overflow(N);<br>+ return SDValue();<br>+}<br>+<br>+SDValue DAGTypeLegalizer::PromoteIntRes_ADDSUBE(SDNode *N, unsigned ResNo) {<br>+ if (ResNo == 1)<br>+ return PromoteIntRes_Overflow(N);<br>+ return SDValue();<br>+}<br><br>why do you return SDValue() if ResNo != 1 ? Shouldn't you either assert or<br>promote the value?<br><br>Ciao, Duncan.<br><br>_______________________________________________<br>llvm-commits mailing list<br><a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br><<a href="mailto:llvm-commits@cs.uiuc.edu">mailto:llvm-commits@cs.uiuc.edu</a>><<a href="mailto:llvm-commits@cs.uiuc.edu">mailto:llvm-commits@cs.uiuc.edu</a>><br><a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br></blockquote><br>_______________________________________________<br>llvm-commits mailing list<br><a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br><<a href="mailto:llvm-commits@cs.uiuc.edu">mailto:llvm-commits@cs.uiuc.edu</a>><<a href="mailto:llvm-commits@cs.uiuc.edu">mailto:llvm-commits@cs.uiuc.edu</a>><br><a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a></blockquote></blockquote></blockquote></blockquote></div></blockquote></div><br></body></html>