RFR: 8241042: x86_64: Improve Assembler generation

[Vladimir Kozlov]

Something messed up. Move prefixq(Address adr, Register src) near corresponding get_prefixq().
Currently an other method Assembler::prefix(Address adr, Register reg, bool byteinst) mixed up in changes.
Which brings question: why you did not do the same for prefix() methods?

May be you can move get_prefixq() changes into separate RFE so we can continue discussion?

Changes emit_int* can be pushed first.

Thanks,
Vladimir

On 3/16/20 4:43 PM, Claes Redestad wrote:
> 
> 
> On 2020-03-16 23:29, Vladimir Kozlov wrote:
>> Okay, emit_int* code change seems fine (I did not verify each and every line - hope testing will show correctness).
> 
> Thanks!
> 
>>
>> About get_prefixq() change.
>>
>> 1. Would be nice to have common prefixes table and method get_prefixq(). Could be done by passing noreg (-1) >
>> void Assembler::prefixq(Address adr) {
>>    emit_int8(get_prefixq(adr, noreg));
>> }
> 
> Hmm, get_prefixq(adr, reg) calls reg->encoding(), which will assert on
> noreg (assert(reg->is_valid(), ...)).
> 
>>
>> 2. Use asserts for them as you did in first get_prefixq() variant.
> 
> This is a good idea, in fact I thought I already had the
> corresponding assert block in both places.
> 
>>
>> 3. May swap values in table by reversion (src->encoding() < 8) condition.
> 
> Hmm, maybe we should just exploit the fact that the REX_W* values
> returned in both cases are consecutive from REX_W to REX_WXB and
> REX_WRXB respectively, and ordered in a way that makes it possible to
> get rid of the lookup tables entirely:
> 
> int8_t Assembler::get_prefixq(Address adr) {
>    int8_t prfx = (int8_t)(REX_W + ((int)adr.base_needs_rex()) + ((int)adr.index_needs_rex() << 1));
>    ..
> 
> int8_t Assembler::get_prefixq(Address adr, Register src) {
>    int8_t prfx = (int8_t)(REX_W + ((int)adr.base_needs_rex()) + ((int)adr.index_needs_rex() << 1) + 
> ((int)(src->encoding() >= 8) << 2));
>    ..
> 
> http://cr.openjdk.java.net/~redestad/8241042/open.02/
> 
> While this is really a bit too clever, the assertions make for a very
> strong fail-safe, and I think all the possibilities should be checked
> rather promply by our regular testing. I've sanity tested locally and
> verified the code gen look a bit better without the lookup tables.
> 
> Running tier1-4 over night.
> 
> Thanks!
> 
> /Claes