FW: RFR (S): Improve adlc usability

[Vladimir Kozlov]

Hi Goetz,

This change works better. I will push it after testing.

Thank you for explaining your problem with constant_offset() during format 
output. May be we should add a flag to check in this method and just return some 
default value when it is called before code generation.

The failures I saw before was on Solaris when building 64-bit VM. And on Windows 
(sign vs unsign compare).

Thanks,
Vladimir

Lindenmaier, Goetz wrote:
> Hi Vladimir, 
> 
> thanks for the fast review!  I'll try to learn for my next changes
> from this.  I'm not always sure what coding style
> to use, whether to keep the diffs small or streamline them with 
> the surrounding code etc.  I also can't tell all the reasons for 
> design decisions in our code, as some changes are rather old
> or were done by colleagues that left our team.
> 
> I fixed what you proposed and just removed what you 
> questioned. 
> 
> About constant_offset_unchecked(): We dump debug info 
> in more phases than your jit does, also calling format() there.
> Actually, in our jit, you can call format() before register allocation. 
> Then it displays node numbers instead of registers.  Therefore
> we made the format() function much more stable. 
> Anyways, constant_offset() has side effects which should
> not be triggered by debug output.  I understand the offset
> is set before the function is called, but then we can output it
> directly, too.
> I remove the function from the change nevertheless.  It's also
> the only place that requires changes to /opto/ and thus fits not 
> that good into this change.
> 
> You find the new webrev here:
> http://cr.openjdk.java.net/~goetz/webrevs/8000592/
> 
> On which platform did you get the build error?
> 
> Best regards,
>   Goetz.
> 
> 
> 
> -----Original Message-----
> From: Vladimir Kozlov [mailto:vladimir.kozlov at oracle.com] 
> Sent: Dienstag, 9. Oktober 2012 03:38
> To: Lindenmaier, Goetz
> Cc: hotspot-compiler-dev at openjdk.java.net
> Subject: Re: RFR (S): Improve adlc usability
> 
> Hi Goetz,
> 
> I would not call it small with 450 lines changed :)
> 
> I filed RFE 8000592: Improve adlc usability
> 
> Review:
> 
> It is not our codding style to have NULL on left side of compare. You have it in 
> several files changes.
> 
> 
> src/share/vm/adlc/archDesc.cpp
> Could you move opForm.dump() under your new check?:
> 
>     if (!result_class) opForm.dump();
> -  assert( result_class, "Resulting register class was not defined for operand");
> +  if (NULL == result_class) {
> 
> 
> src/share/vm/adlc/formssel.cpp:
> I don't think next code will work with num_opnds() == 0. Move num_opnds() check 
> before the loop:
> 
> +const char *InstructForm::unique_opnd_ident(int idx) {
> +  uint i;
> +  for (i = 1; (num_opnds() >= 0) && (i < (uint)num_opnds()); ++i) {
> +    if (unique_opnds_idx(i) == idx) {
> +      break;
> +    }
> +  }
> +  return (_components.at(i)) ? _components.at(i)->_name : "";
> +}
> 
> src/share/vm/adlc/formssel.cpp:
> Why do you need separate static method use_def_effect_name()? It is called only 
> from one place.
> 
> src/share/vm/adlc/formssel.cpp:
> I don't get why you need special constant_offset_unchecked() for format output. 
> We dump format (PrintOptoAssembly) after code is generated so all constants 
> offsets should be defined. What asserts you have "in debug output"?
> 
> 
> src/share/vm/adlc/main.cpp:
> You missed -v<GLOBALS_FILE_NAME> description:
> 
>     printf(" v  specify adGlobals output file name\n");
> 
> 
> src/share/vm/adlc/output_h.cpp
> We can't accept comment with "// SAPJVM ..."
> 
> Don't do assignment inside condition:
> 
>   134     for ( _register->reset_RegDefs(); (reg_def = 
> _register->iter_RegDefs()) != NULL; ) {
> 
> And I got next build failures with your changes:
> 
> src/share/vm/adlc/output_h.cpp(86) : warning C4018: '<' : signed/unsigned mismatch
> 
> "src/share/vm/adlc/output_h.cpp", line 135: Error: Conversion of 64 bit type 
> value to "int" causes truncation.
> "src/share/vm/adlc/output_h.cpp", line 145: Error: Conversion of 64 bit type 
> value to "int" causes truncation.
> 2 Error(s) detected.
> 
> 
> Regards,
> Vladimir
> 
> Lindenmaier, Goetz wrote:
>> Hi,
>>
>>  
>>
>> We did a row of smaller changes to adlc to improve its usability, e.g.
>>
>> - Make adlc more stable, especially if parsing faulty .ad files.
>>
>> - In some cases raise syntax errors instead of asserting.
>>
>> - Write more comments into generated files and beautify format.
>>
>> - Make calls to node->format() more stable.
>>
>> - Clean up adlc code: warnings, dead code etc.
>>
>>  
>>
>> Notice that all these changes shouldn't affect the generated code in any 
>> way!
>>
>>  
>>
>> I collected all these and put them into this webrev:
>>
>>  http://cr.openjdk.java.net/~goetz/webrevs/webrev-adlc_usability/
>>
>>  
>>
>> We would like to contribute this to the openJDK.  Could somebody
>>
>> please review the change and push it if it seems useful to you?
>>
>>  
>>
>> Thanks a lot,
>>
>>   Goetz.
>>
>>  
>>