On 7/25/14 12:18 AM, Tobias Hartmann wrote:
> Hi Vladimir,
>
> thanks for the review.
>
> On 24.07.2014 17:48, Vladimir Kozlov wrote:
>> 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.
>
> Yes, sentence 13 of Section 9.2. (page 217) of the latest C++ standard working draft [1] says:
>
> "Nonstatic data members of a (non-union) class with the same access control (Clause 11) are allocated so that later
> members have higher addresses within a class object. The order of allocation of non-static data members with different
> access control is unspecified (Clause 11). [...]"
>
> We do not have any access control specifiers, so it should be fine.
Good. Thank you for looking to this.
Vladimir
>
> Thanks,
> Tobias
>
> [1] https://isocpp.org/files/papers/N3797.pdf
>
>> > 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
>>>>>
>>>
>