[9] RFR(S): 8049043: Load variable through a pointer of an incompatible type in hotspot/src/share/vm/runtime/sharedRuntimeMath.hpp
Vladimir Kozlov
vladimir.kozlov at oracle.com
Thu Jul 24 15:48:03 UTC 2014
>> Or you can use union with jlong and don't mess with ENDIAN at all. 64-bit is now main platform now.
>>
>> Vladimir
>
> How do we access the low and high bits then?
Shifts and masks as we do in other places.
Do we trust that C++ compilers keep declared order of int fields? Is it in spec? If yes I am fine with your last code.
> typedef union {
> } DoubleIntConv;
> But that does not really improve code readability... What do you think?
It is fine, we do that.
> I refactored the code and added set_high(..) and set_low(..) methods (similar to the ones defined in
globalDefinitions.hpp).
>
> New webrev: http://cr.openjdk.java.net/~thartmann/8049043/webrev.03
I like set_* methods. This webrev.03 looks good.
Thanks,
Vladimir
On 7/24/14 2:47 AM, Tobias Hartmann wrote:
> Hi,
>
> Vladimir, David, thanks for the review. Please see comments inline.
>
>>> Tobias, why you are creating local copy 'conv' of a variable in all places?
>
> Doing it like this
>
> __HI(x) = (__HI(x)&0x7fffffff)|(__HI(y)&0x80000000);
> return x;
>
> Parfait still complained that a 32 bit value is assigned to x and it is then used as a 64 bit value. That's why I had to
> create a local copy of 'conv'.
>
> On 23.07.2014 21:30, David Chase wrote:
>> Vladimir, I think the mistake is instead to include the cast to the type that it already is.
>
> True, the cast is not necessary.
>
> On 23.07.2014 23:12, Vladimir Kozlov wrote:
>> Or you can use union with jlong and don't mess with ENDIAN at all. 64-bit is now main platform now.
>>
>> Vladimir
>
> How do we access the low and high bits then?
>
>> 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.
>
> I like this solution because in my opinion it is the most readable one and it is always a good idea to remove ugly
> macros. If the potential slowdown in slowdebug builds is not a problem I would like to stick to this.
>
> I removed the macros and changed all parts of the code that used them (unfortunately quite a lot).
>
> The parfait warnings in the files sharedRuntimeMath.hpp, sharedRutimeTrans.cpp and sharedRuntimeTrigg.cpp were reduced
> by 12.
>
> New webrev: http://cr.openjdk.java.net/~thartmann/8049043/webrev.02
>
> The problem with this solution is that some of the compilers do not support anonymous structs. We could name the struct
> and access it via the name, for example like this:
>
> typedef union {
> double d;
> struct {
> #ifdef VM_LITTLE_ENDIAN
> int lo;
> int hi;
> #else
> int hi;
> int lo;
> #endif
> } i;
> } DoubleIntConv;
>
> DoubleIntConv conv = { x };
> int high = conv.i.hi;
>
> But that does not really improve code readability... What do you think?
>
> Thanks,
> Tobias
>
>>> (Aren’t you loving this review process? :-)
>
> Yes, thanks for all the testing :)
>
> Best,
> Tobias
>
>>>
>>> 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
>>>
>
More information about the hotspot-compiler-dev
mailing list