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

[Max Ockner]

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.

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
>>
>