RFR: 8013393: Merge templateTable_x86 32 and 64 bit files into one file.

[serguei.spitsyn at oracle.com]

On 3/4/15 9:16 PM, Max Ockner wrote:
> Serguei-
> I originally tested with JPRT,  vm.quick.testlist,  and 
> nsk.jvmti.testlist.  After making formatting changes, I retested with 
> nsk.jvmti.testlist. Still, just to be safe, I also retested with 
> vm.quick.testlist today.

Good.

Thanks, Max!
Serguei

>
> John-
> The _new operator used registers that weren't available on 64 bits, so 
> the 32 bit version was kept.
>
> Thanks,
> Max
>
> On 3/3/2015 7:00 PM, serguei.spitsyn at oracle.com wrote:
>> Hi Max,
>>
>> A minor comment.
>> The testlist (nsk.jvmti.testlist) looks strange.
>> I'd expect at least some runtime tests to be run.
>>
>> Thanks,
>> Serguei
>>
>>
>> On 3/3/15 1:39 PM, Max Ockner wrote:
>>> Hello all,
>>> I have made all of the changes recommended by Chris and John. I 
>>> realize there are plenty of further cleanups and optimizations that 
>>> I could make, but I think this is nearing a good stopping point for 
>>> now. We can always clean it up more later. What do you think?
>>>
>>> new webrev:   cr.openjdk.java.net/~coleenp/8013393
>>> tested with nsk.jvmti.testlist
>>>
>>> On 3/2/2015 4:45 PM, John Rose wrote:
>>>> On Mar 2, 2015, at 11:46 AM, Christian Thalinger 
>>>> <christian.thalinger at oracle.com> wrote:
>>>>> That’s quite a large change (I wish we had Crucible or something 
>>>>> to leave inline comments).
>>>>>
>>>>> First, you have to keep the oldest copyright year (but maybe 
>>>>> that’s just the diff).
>>>>>
>>>>> + #ifdef _LP64
>>>>> + void TemplateTable::lconst(int value) {
>>>>>
>>>>> I’d prefer to have #ifdef’s inside methods and not around them.  
>>>>> There are a bunch; please change all of them.
>>> I have moved the #ifdef's to the inside of the function 
>>> declarations. Some of the #ifdefs could be moved far enough in that 
>>> it made sense to replace them with the LP64_ONLY() or NOT_LP64() 
>>> tools. The remaining #ifdef's are there because some large sections 
>>> of code could not be merged, and needed to be kept together.
>>>
>>>> I agree Christian's comments, and especially with this request.
>>>>
>>>> A few more points on small matters:
>>>>
>>>> In our source base, C++ comments usually begin with two slashes and 
>>>> a space (less than 10% exceptions).
>>>>    +//Global Register Names
>>>>    +//Address Computation: local variables
>>>>    (etc.)
>>>
>>> Fixed.
>>>
>>>> Let's use the term "amd64" only where required; x86_64 is the 
>>>> standard now.
>>>>     // No amd64 specific initialization
>>>
>>> Fixed.
>>>
>>>> More on #ifdefs:  I think there are too many; Chris has pointed out 
>>>> the main problem.
>>>>
>>>> It's a judgement call, but here's another example:
>>>>
>>>> +#ifdef _LP64
>>>> +        Register rtmp = r8;
>>>> +        Register rthread = r15_thread;
>>>> +#else
>>>> +        Register rtmp = rsi;
>>>> +        Register rthread = rcx;
>>>> +        __ get_thread(rcx);
>>>> +        __ save_bcp();
>>>> +#endif // _LP64
>>>>
>>>> could just as well be:
>>>>
>>>> +        Register rtmp = LP64_ONLY(r8) NOT_LP64(rsi);
>>>> +        Register rthread = LP64_ONLY(r15_thread) NOT_LP64(rcx);
>>>> +        LP64_ONLY(__ get_thread(rcx));
>>>> +        LP64_ONLY(__ save_bcp());
>>>>
>>>> Advantages:  Fewer lines, less duplication, less chance of merge 
>>>> problems.
>>>> Possible objection:  Ugly; but only in a different way from the 
>>>> other ugly.
>>>
>>> Fixed, but there are some #ifdef's left over. There are some pretty 
>>> large code blocks that are conditionally defined. I don't think it 
>>> makes sense to enclose every conditionally defined line of code in 
>>> an LP64_ONLY or NOT_LP64. At best you save 3 lines of code, but I 
>>> feel that a those 3 lines would be less distracting than having 25 
>>> LP64_ONLY's in a row.
>>>>
>>>> However, I admit that is a marginal example.
>>>>
>>>> — John
>>>
>>> Thanks,
>>> Max Ockner
>>>
>>
>