review (M) for 6892658: C2 should optimize some stringbuilder patterns

[Tom Rodriguez]

On Nov 11, 2009, at 12:08 PM, Vladimir Kozlov wrote:

> Final part.
> 
> You use kit.gvn(). instead of _gvn-> in several places.
> Also I think you can use __ instead of kit. for intcon, makecon, loads
> and others and leave it for control, memory operations only.
> 
> You use fetch_static_field() only to read Integer.sizeTable. Does it need
> to be so generalized? But you can keep it as it is.

Originally I was going to read some other fields so I needed something general.  It's based on do_get_xxx and I don't see any reason to simplify it.  I could move it over into GraphKit.

> 
> Can you separate inlined comments from code by empty lines in int_stringSize()?

Ok.

> In int_stringSize(), I think, TypeAryPtr::INTS memory should be used instead of
> TypeAryPtr::CHARS (for final_mem) and int_adr_idx (needs to add it) instead
> of char_adr_idx.

Actually there are no stores so it's not needed at all.  I'd added some debugging code that did a runtime call and needed the phi but I think I can remove it completely now.

> 
> In int_getChars() should the sign store to have IfTrue(iff) control?:

Ah yes.  It should.  The store in the loop above has a similar problem.

> 
> 1124     Node* st = __ store_to_memory(kit.control(), kit.array_element_address(char_array, m1, T_CHAR),
> 1125                                   sign, T_CHAR, char_adr_idx);
> 1126
> 1127     final_merge->init_req(1, __ IfTrue(iff));
> 
> 
> copy_string(), so you not supporting byte array strings for now?

We don't have byte strings.

> Why 6 is limit for constant strings? Add some comments.

Ok. It's just a number.  6 seems like an ok code space vs. speed tradeoff.

tom

> 
> 
> Vladimir
> 
> Tom Rodriguez wrote:
>> http://cr.openjdk.java.net/~never/6892658/