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

[Chris Lattner]

>> Also, please try to stick with the established style when modifying
>> existing code.  In this case, putting the '*' in the right place,
>> capitalizing variables, etc.
>>
> Old habits die hard. I've had it drilled into my head for decades that
> putting the * next to the var name is the *wrong* place to put it  
> as the
> * modifies the type not the var. I'll try to retain style consistency,
> however.

I agree with the theoretical idea behind that style, but... :)

>> The cast sequence *is* needed for converting stuff to shifts etc,
>> above.
>
> I'm not following this comment. What cast sequence? The one where  
> we're
> trying to turn it into a udiv if the sign bits are zero?  If so, that
> contradicts your simplification for this transform.

The specific transform this was attached to get simpler (no casts)  
but others get temporarily nastier (new casts) until other operations  
become signless.

>> in the commonIDivTransforms method, but it is specific to udiv.
>> Please move it to the udiv case, and make it insert casts etc as
>> needed.
>
> Actually, I think I lost it completely. I don't see this in my file. I
> added it back to the udiv case as a separate transform. It now checks
> for the STO==0 and SFO==0 cases because they are no longer "handled
> above" (the "above" code is in commonDivTransforms).

Ok.

>> *** This is subtly buggy.  If you look at how MulWithOverflow is  
>> used,
>> it is called from the "Fold: (div X, C1) op C2 -> range check" case.
>> You need to pass in whether or not the *operation* is signed or
>> unsigned, ignoring the sign of the values.
>
> MulWithOverflow isn't used elsewhere in the file so I merged it into
> visitSelectCC so its a little more clear what's going on. No point
> making a function with 4 arguments called from only one place.

Ok, though sometimes names do give useful info to the reader.  A  
comment does just the same though.

Thx Reid,

-Chris