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

[Coleen Phillimore]

Hi Dean,
I'm not sure how to do this diff that you're suggesting.  I don't think 
an intermediate changeset makes sense either. The bytecodes are pretty 
well tested with our normal testing so increasing this to more than one 
change, just increases testing and putback overhead.

I've looked at this several times and it's not very difficult to tell 
what's done and whether it's correct.  The diffs do not span a lot of 
code (at most 5 lines at a time), so there are no new whole functions to 
read that are new code.

I'm still really don't understand what kind of diff you're requesting 
that would make it easier.  Can you provide an example?

thanks,
Coleen

On 2/27/15, 6:13 PM, Dean Long wrote:
> Hi Coleen.  I believe I understand what Max did, and that it was not 
> an automated merge,
> which to me says that properly reviewing the change requires looking 
> at all 2851
> changed lines and mentally deciding if the result is equivalent to the 
> original.
>
> What I was suggesting was essentially two things.  First, an aid to 
> the reviewer,
> and second, a way to reduce risk.  In the end it's up to the 
> reviewers.  If they
> prefer to see the changes in a single diff, they can compare a 
> single-file
> automated merge with the single-file result of Max's merge.  It 
> doesn't matter
> if the reviewer does it or Max does it as a convenience. Regarding risk,
> if the reviewers aren't comfortable deciding that all 2851 are correct,
> the changes could be staged into several pieces, and the first piece 
> could
> be an automated merge that would be cleaned up later, on the assumption
> that an automated merge could be considered trivially correct and 
> therefore
> not risky.  I hope that clears up my somewhat terse comments below.
>
> dl
>
> On 2/27/2015 12:06 PM, Coleen Phillimore wrote:
>>
>> Dean,  This merge wasn't automatic in any way, so I don't think what 
>> you're proposing makes sense.  I think if you look at the webrev 
>> you'll see what Max has done.
>> Thanks,
>> Coleen
>>
>> On 2/27/15, 1:39 PM, Dean Long wrote:
>>> Hi Max.  You could do an automatic merge using something like "diff 
>>> -w -D LP64 templateTable_x86_32.cpp and templateTable_x86_64.cpp", 
>>> then compare
>>> your improvements to that.  That way all the changes would show up in
>>> a single diff.  Or you could even check in the automatic merge 
>>> first, to reduce
>>> risk, then break up the improvements into smaller pieces.
>>>
>>> dl
>>>
>>> On 2/27/2015 9:36 AM, Max Ockner wrote:
>>>> Hello all,
>>>> Please review this change which involves merging 32 and 64 bit 
>>>> interpreter files.
>>>>
>>>> Bug ID: 8013393
>>>> Webrev: cr.openjdk.java.net/~coleenp/8013393
>>>>
>>>> Summary: templateTable_x86_64.cpp and templateTable_x86_32.cpp have 
>>>> been merged into one file*. These files originally shared thousands 
>>>> of lines of duplicate code.
>>>> There were also many nearly identical sections of code which 
>>>> differed only in register usage (example: rsi vs. r13) or in the 
>>>> usage of nearly identical functions (example: movptr vs. movq/movl).
>>>> Given that (1) new bytecodes could be added soon, and (2) these 
>>>> files have been tormenting Coleen for years, this change seems 
>>>> overdue.
>>>>
>>>> *There are currently two files. The updated templateTable_x86.cpp 
>>>> is copied into both templateTable_x86_32.cpp and 
>>>> templateTable_x86_64.cpp to make it easier to review (so the diff 
>>>> is not the entire file). This will be changed back to one file 
>>>> after review.
>>>>
>>>> For functions that could be merged, one copy was kept. In some 
>>>> cases, files were merged by factoring out small differences such as 
>>>> register usage (example: rbcp equals r13 or rsi depending on the 
>>>> platform). For functions that could not be merged, the 32 and 64 
>>>> bit versions are adjacent and are defined conditionally using 
>>>> #ifdef based on the platform.
>>>>
>>>> There are still improvements to be made, but they are small and can 
>>>> be filed seperately.
>>>>
>>>> Tested with:
>>>> JPRT
>>>> UTE nsk.jvmti.testlist
>>>>
>>>>
>>>> Thanks,
>>>> Max Ockner
>>>
>>
>