RFR: 8013393: Merge templateTable_x86 32 and 64 bit files into one file.
Dean Long
dean.long at oracle.com
Fri Feb 27 23:13:21 UTC 2015
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
>>
>
More information about the hotspot-dev
mailing list