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

[Dean Long]

OK.  If we had to take as much time to review the "diff -D" merge as
the human merge, then it's probably not worth it.  It's only if we trust
the "diff" merge and use that as a starting point for the pretty human
merge that I see it helping.

dl

On 3/2/2015 12:30 PM, Coleen Phillimore wrote:
>
> Cool option to diff.  I didn't know that existed.  In this case, it 
> would yield a file that would be mostly impossible to review. Max's 
> change goes a lot further in merging these files, and would be 
> unfortunate to go backwards.  This diff would leave a mess in the file 
> with a lot of #ifdefs that were easily resolved with register names 
> that I wouldn't want to look at.
>
> Coleen
>
> On 3/2/15, 1:37 PM, Dean Long wrote:
>> Hi Coleen.  Let's say the _32 file contains
>>
>> 32
>> x
>> y
>> z
>> int
>>
>> and the _64 file contains
>>
>> 64
>> x
>> y
>> z
>> long
>>
>> Then "diff -DLP64 _32 _64" will give:
>>
>> #ifndef LP64
>> 32
>> #else /* LP64 */
>> 64
>> #endif /* LP64 */
>> x
>> y
>> z
>> #ifndef LP64
>> int
>> #else /* LP64 */
>> long
>> #endif /* LP64 */
>>
>> In other words, an automatic merge of the two files, with the 
>> differences in #ifdefs.
>>
>> thanks,
>>
>> dl
>>
>> On 3/2/2015 7:01 AM, Coleen Phillimore wrote:
>>>
>>> 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
>>>>>>
>>>>>
>>>>
>>>
>>
>