>
>> 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
_______________________________________________
llvm-commits mailing list
llvm-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
