[PATCH] [Multiprecision Arithmetic Optimization] Teach SelectionDAG how to promote the integer carry result of {add, sub}{c, e} to a larger integer type.

[Michael Gottesman]

Hey Duncan. I am going to respond in a backwards order since I think it will yield additional clarity [if it does not, please tell me and I will elaborate = )]:

> Actually, isn't ADDC really exactly the same as UADDO?  If so, how about
> eliminating the ADDC in favour of UADDO?  In any case, there's already
> promotion logic for UADDO, DAGTypeLegalizer::PromoteIntRes_UADDSUBO, so
> maybe you could take a look.


While ADDC/UADDO seem similar they are meant to model two different things.

UADDO is meant to model the situation where one performs an addition and wishes to detect overflow as a boolean value and perform some non-arithmetic operation, not perform additional arithmetic with said overflow value. Thus it makes sense when we are promoting UADDO into an add + setne operation since UADDO is lowered into similar code anyways (see LowerXALUO X86ISelLowering.cpp) meaning no performance hit.

On the other hand ADDC is meant to model the situation where one wishes to perform additional arithmetic with the overflow value (specifically in a multiprecision arithmetic context), i.e. on intel:

	addq (%rbx), %rax
	adcq 8(%rbx), %rcx
	adcq 16(%rbx), %ddx
	...

If we were to lower ADDC like UADDO, we would instead have something along the lines of the following non-optimal codegen:

	xorq %rax, %rax
	addq (%rbx), %rcx
	setne %al
	movq %rax, %r8
	addq 8(%rbx), %r9
	getne %al
	addq %r9, %r8
	...

Thus the necessity of this distinction. X86 definitely follows said distinction if you look at the lowering.

Which leads me to your first paragraph.

> thanks for adding the ResNo == 0 case, but I don't think it is correct.
> That's because you continue to return an ADDC code, yet because you zero
> extended the add operands there is no reason why the extended operands
> should overflow (set the carry flag) exactly when the original operation
> would.


This is not completely true. It depends on how you do it. Specifically it depends on how one legalizes the source operations of the addc. For instance if one is loading from an 8 bit array, zext to a 32 bit operation and then perform the addc you would have the issue you are thinking of. In contrast, if you could guarantee that the memory access was promoted to 32 bit (since addition of multiprecision operations with different word sizes used yields the same result), one would no longer have that issue. On the other hand, I do not think I can assume this will be done and it is irrelevant to the discussion since we are discussing PromotingIntegerResults = p.

Given that we are talking about promoting integer results, perhaps the proper way to do this is to just zero extend ResNo == 0 (i.e. not the operands, just the result). What do you think Duncan?

Michael

P.S. I realized the reason why I returned SDValue was b/c I thought the integer promotion was like DAGCombine where one returns SDValue if one is not doing anything vs asserting in PromoteIntegerResult where it looks like you assert. = (.

On May 21, 2013, at 12:12 PM, Duncan Sands <duncan.sands at gmail.com> wrote:

> Hi Michael,
> 
> On 17/05/13 23:33, Michael Gottesman wrote:
>> Hows this look Duncan/Sean?
> 
> 
> I didn't think about ADDE but at a glance it has the same flaw.
> 
> Ciao, Duncan.
> 
>> 
>> 
>> 
>> 
>> Michael
>> 
>> On May 16, 2013, at 10:41 AM, Michael Gottesman <mgottesman at apple.com
>> <mailto:mgottesman at apple.com>> wrote:
>> 
>>> I am trying to be incremental and did not have a need for such lowering. On
>>> the other hand if you want I can implement that in this patch. Just state your
>>> preference = ).
>>> 
>>> Michael
>>> 
>>> On May 16, 2013, at 1:34 AM, Duncan Sands <duncan.sands at gmail.com
>>> <mailto:duncan.sands at gmail.com>> wrote:
>>> 
>>>> Hi Michael,
>>>> 
>>>> On 15/05/13 19:37, Michael Gottesman wrote:
>>>>> The attached patch teaches selection DAG how to promote the integer carry
>>>>> result of an {add,sub}{c,e} node to a larger integer type. It is a part of a
>>>>> larger set of patches that I will be committing over the next little bit to
>>>>> enable the optimization of uaddo/usubo chains.
>>>>> 
>>>>> Please Review,
>>>>> Michael
>>>> 
>>>> +SDValue DAGTypeLegalizer::PromoteIntRes_ADDSUBC(SDNode *N, unsigned ResNo) {
>>>> +  if (ResNo == 1)
>>>> +    return PromoteIntRes_Overflow(N);
>>>> +  return SDValue();
>>>> +}
>>>> +
>>>> +SDValue DAGTypeLegalizer::PromoteIntRes_ADDSUBE(SDNode *N, unsigned ResNo) {
>>>> +  if (ResNo == 1)
>>>> +    return PromoteIntRes_Overflow(N);
>>>> +  return SDValue();
>>>> +}
>>>> 
>>>> why do you return SDValue() if ResNo != 1 ?  Shouldn't you either assert or
>>>> promote the value?
>>>> 
>>>> Ciao, Duncan.
>>>> 
>>>> _______________________________________________
>>>> 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
>>> 
>>> _______________________________________________
>>> 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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130521/ca350bb0/attachment.html>