[9] RFR(S): 8049043: Load variable through a pointer of an incompatible type in hotspot/src/share/vm/runtime/sharedRuntimeMath.hpp

[Vladimir Kozlov]

Or you can use union with jlong and don't mess with ENDIAN at all. 
64-bit is now main platform now.

Vladimir

On 7/23/14 1:00 PM, David Chase wrote:
> It occurred to me that a cleaner way is to put the ifdef in the definition of the union:
>
> typedef union {
>     double d;
>     struct {
> #ifdef VM_LITTLE_ENDIAN
>       int lo;
>       int hi;
> #else
>       int hi;
>       int lo;
> #endif
>     };
> } DoubleIntConv;
>
> # define __HI(x) (x.hi)
> # define __LO(x) (x.lo)
>
> At which point, the continued use of the macro starts to look perhaps a hair silly.
>
> I was inspired to write the following test program and compile it under clang and gcc (but not under Solaris Studio yet) to see if we could instead just write a stinking static method (that would be inlined) and at clang -O3 and gcc -O4, it was indeed inlined.  So we could just write clean code and to heck with the macro.  (Aren’t you loving this review process? :-)
>
> We would, however, suffer some slowdown in slowdebug builds.
> ----------------------
> #define VM_LITTLE_ENDIAN 1
> typedef union {
>     double d;
>     struct {
> #ifdef VM_LITTLE_ENDIAN
>       int lo;
>       int hi;
> #else
>       int hi;
>       int lo;
> #endif
>     };
> } DoubleIntConv;
>
> # define __HI(x) (x.hi)
> # define __LO(x) (x.lo)
>
> static int inline_low(double d) {
>    DoubleIntConv x = { d } ;
>    return __LO(x);
> }
>
> static int inline_high(double d) {
>    DoubleIntConv x = { d } ;
>    return __HI(x);
> }
>
> int low(double d) {
>    return inline_low(d);
> }
>
> int high(double d) {
>    return inline_high(d);
> }
> ----------------------
>
>
>
> On Jul 23, 2014, at 3:37 PM, Vladimir Kozlov <vladimir.kozlov at oracle.com> wrote:
>
>> David, I agree with your suggestion to pass union to _HI and _LO. The creation of a local union var makes sense then.
>>
>> Thanks,
>> Vladimir
>>
>> On 7/23/14 12:30 PM, David Chase wrote:
>>> Vladimir, I think the mistake is instead to include the cast to the type
>>> that it already is.
>>> I think performing the cast to the union type from a non-union address
>>> might get you a warning otherwise.
>>> I can confirm that if you write it plainly with the assignment to a
>>> correctly typed “conv”
>>> that clang (at least) generates good code:
>>>
>>> union ca {
>>>     double d;
>>>     int i[2];
>>> };
>>>
>>> int la(double d) {
>>>    union ca x = { d } ;
>>>    return x.i[0];
>>> }
>>>
>>> clang produces (-O3 -S, cleaned up)
>>>
>>> _la:                                    ## @la
>>>          pushq   %rbp
>>>          movq    %rsp, %rbp
>>>          movd    %xmm0, %rax
>>>          popq    %rbp
>>>          ret
>>> gcc 4.8.1 produces (-O4 -S, cleaned up)
>>>
>>> la:
>>> movsd%xmm0, -8(%rsp)
>>> movq-8(%rsp), %rax
>>> ret
>>>
>>> So I think this is generally a good idea, but the macro should be
>>> cleaned up to
>>> remove the cast that is certainly unnecessary now.  E.g.
>>>
>>>    39 #ifdef VM_LITTLE_ENDIAN
>>>    40 # define __HI(x) ((x).i[1])
>>>    41 # define __LO(x) ((x).i[0])
>>>    42 #else
>>>    43 # define __HI(x) (x.i[0])
>>>    44 # define __LO(x) (x.i[1])
>>>    45 #endif
>>>
>>> The temporary is only annoying to humans using a last-millenium dialect
>>> of C.  The code runs fast, and the address-of+dereference in the macro
>>> forced it to memory (lacking a lot of alias analysis) anyhow.
>>>
>>> David
>>>
>>>
>>> On Jul 23, 2014, at 1:58 PM, Vladimir Kozlov <vladimir.kozlov at oracle.com
>>> <mailto:vladimir.kozlov at oracle.com>> wrote:
>>>
>>>> Tobias, why you are creating local copy 'conv' of a variable in all
>>>> places?
>>>>
>>>> Vladimir
>>>>
>>>> On 7/23/14 4:12 AM, Tobias Hartmann wrote:
>>>>> Hi,
>>>>>
>>>>> please review the following patch for JDK- 8049043 (Parfait warning).
>>>>>
>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8049043
>>>>> Webrev: http://cr.openjdk.java.net/~thartmann/8049043/webrev.01/
>>>>>
>>>>> Problem:
>>>>> Parfait issues 6 warnings " Load variable through a pointer of an
>>>>> incompatible type" because of the __HI(x) and __LO(x) macros used in
>>>>> copysignA and scalbnA (share/vm/runtime/sharedRuntimeMath.hpp) to cast
>>>>> between double* and int*.
>>>>>
>>>>> Solution:
>>>>> A union 'DoubleIntConv' is used to access the high and low 32 bits of a
>>>>> double.
>>>>> The '# pragma warning( disable: 4748 )' in sharedRuntimeTrans.cpp is
>>>>> necessary because otherwise the VS compiler fails with the following
>>>>> warning: "/GS can not protect parameters and local variables from local
>>>>> buffer overrun because optimizations are disabled in function". There
>>>>> exists a bug to fix this [1].
>>>>>
>>>>> Testing:
>>>>> JPRT, Parfait (all 6 warnings removed)
>>>>>
>>>>> Thanks,
>>>>> Tobias
>>>>>
>>>>> [1] https://bugs.openjdk.java.net/browse/JDK-8046696
>