[9] RFR(S): 8049043: Load variable through a pointer of an incompatible type in hotspot/src/share/vm/runtime/sharedRuntimeMath.hpp
Tobias Hartmann
tobias.hartmann at oracle.com
Thu Jul 24 11:55:28 UTC 2014
Hi,
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
Testing: Parfait, JPRT
Thanks,
Tobias
On 24.07.2014 11:47, 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
>>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20140724/4ace5704/attachment-0001.html>
More information about the hotspot-compiler-dev
mailing list