FW: RFR (S): Improve adlc usability

[Lindenmaier, Goetz]

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