On Oct 21, 2014, at 1:31 PM, Marcus Lagergren <[email protected]> 
wrote:

> An example in this comment would have helped me understand this sooner. Maybe 
> you can add one? It would be appreciated.
> +                // Can reorder, but can't move conversion into the operand 
> as the operation depends on operands
> +                // exact types for its overflow guarantees.

Modified it to:

// Can reorder, but can't move conversion into the operand as the operation 
depends on operands
// exact types for its overflow guarantees. E.g. with {L}{%I}expr1 {L}* 
{L}{%I}expr2 we are not allowed
// to merge {L}{%I} into {%L}, as that can cause subsequent overflows; test for 
JDK-8058610 contains
// concrete cases where this could happen.

> Can these be private?

They can, but then javac will add synthetic package-private accessors for 
access from CodeGenerator$TypeBounds. Matter of style, FWIW. I'll "privatize" 
them.
 
> 
> +    static Type booleanToInt(final Type t) {
> +        return t == Type.BOOLEAN ? Type.INT : t;
> +    }
> +
> +    static Type objectToNumber(final Type t) {
> +        return t.isObject() ? Type.NUMBER : t;
> +    }
> 
> Wasn’t it possible by solving this by always using an indy lmul(jj)j that 
> might throw an UnwarrantedOptimismException containing a double, or is it too 
> expensive to introduce the 53 bit check logic there?

Yes, it is possible (I say as much in this comment: 
<https://bugs.openjdk.java.net/browse/JDK-8058610?focusedCommentId=13566721&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13566721>),
 but it'd require a more extensive modification of code generator that I'd 
rather not embark on now. Namely, CodeGenerator.OptimisticOperation has an 
elaborate criteria for deciding whether it should indeed be optimistic or not 
(see setting of the isOptimistic flag in its constructor), and it'd have to 
accommodate the special case of BinaryNodes for arithmetic operations then.

> Are we using lmul(jj)j elsewhere and not checking this?

No. CodeGenerator's loadMUL and loadASSIGN_MUL are the only cases, and they're 
covered. FWIW, the tests exercise ADD, SUB, and MUL operations, as this was not 
constrained to multiplication only. Division and modulo are unaffected as they 
were treated as special cases anyway, as even without overflowing their results 
can lead out of the set of integral numbers into floating point numbers.

> 
> Otherwise +1
> 
> Regards
> Marcus
> 
> 
> On 21 Oct 2014, at 14:46, Attila Szegedi <[email protected]> wrote:
> 
>> Please review JDK-8058610 at 
>> <http://cr.openjdk.java.net/~attila/8058610/webrev.00> for 
>> <https://bugs.openjdk.java.net/browse/JDK-8058610>
>> 
>> Thanks,
>>  Attila.
> 

Reply via email to