[llvm-commits] DIV -> U/S/FDiv Patch For Review

[Chris Lattner]

>
>> It would be more clear to return 0 here explicitly and mention in the
>> method comment that this returns null if no upgrading is needed.
>
> Except that's not quite the case. The function can return 0 but alter
> the OpCode argument.  I'll fix the comment to explain in more detail.

Great, thx.

>> *** This is flat out incorrect.  It should have separate
>> execute*DivInst functions that do the operation not based on the  
>> type.
>> This would misinterpret 'udiv int -4, 2' for example. This must be
>> fixed before you checkin.
>>
> Another casualty of thinking the operand types had to match the
> instruction type. I've created the three functions. The SDiv case now
> looks like:
>
> static GenericValue executeSDivInst(GenericValue Src1, GenericValue
> Src2,
>                                    const Type *Ty) {
*snip*

Looks good.

>> *** The CBE has the same problem.  You need to force the operands to
>> the right sign in the C output so that the C compiler will generate
>> the appropriately signed operation.  This must be fixed before you
>> checkin.
>
> Yes. Done. I added a writeOperandWithCast(Value*, unsigned opcode)
> method to the CWriter. It writes the operand with the correct cast  
> based
> on the opcode the value is used with. I call this instead of
> writeOperand(Value*) to write the operands for these binary operators
> (just before and after the switch statement you quoted above).

Sounds good.  Make sure that the result also ends up as the right  
type though.

>> You properly sign/zero extend the values here, but then proceed to do
>> the wrong division.  For example, this will do signed division for
>> 'udiv int -2, 2'.  What does the -constfold pass produce for:
>>
>>
>> int %test() { %X = udiv int -2, 2 ret int %X }
>>
>>
>> I bet it is folded to -1, which is incorrect.
>
> There's no -constfold pass on opt, but I ran it through gccas and yes,
> it does produce -1. I fixed it by changing:

Sorry, -constprop.

> BuiltinType R =
>>  (BuiltinType)V1->getSExtValue() / (BuiltinType)V2->getSExtValue();
>
> to:
>
> BuiltinType R = (BuiltinType)(V1->getSExtValue() / V2->getSExtValue 
> ());
>
> That produced the value of 2147483647 ((MAX_UINT-1)/2) for your test
> case which I think is correct.  Please confirm.

Sounds right.  getSExtValue returns a signed type, forcing a signed  
operation.  You do the same for udiv, right?



>>  Constant *ConstantExpr::getAnd(Constant *C1, Constant *C2) {
>>
>>
>> *** This would be an excellent place to assert that the arguments are
>> integer or fp (or vectors of) as appropriate.
>
> All of these cases call ConstantExpr::get(Opcode, C1, C2). That  
> function
> provides copious asserts. I didn't think it was reasonable to double
> them up, especially since the asserts in ConstantExpr::get are ifdef'd
> for debug only. I took that to mean that they were performance  
> sensitive
> for a release+asserts build.  Let me know if that's not the case (i.e.
> if I should remove the #ifndef DEBUG code in ConstantExpr::get).
>
> I looked at ConstantExpr::get and tightened up the asserts there.  
> It was
> allowing FP for UDiv and SDiv and integer for FDiv. Now it doesn't.

Sounds good!

-Chris