[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-compiler-dev
Subject:    Re: RFR(xs): java.lang.Class.isPrimitive() (C1) returns wrong result if Klass* is aligned to 32bit
From:       Thomas_Stüfe <thomas.stuefe () gmail ! com>
Date:       2019-10-29 10:11:06
Message-ID: CAA-vtUy6U_Ecq8s1sfXUyCvruspr4BKgiAuzQGaKpoLFbG9xww () mail ! gmail ! com
[Download RAW message or body]

Hi Andrew,

On Tue, Oct 29, 2019 at 10:32 AM Andrew Haley <aph@redhat.com> wrote:

> On 10/26/19 8:10 AM, Thomas Stüfe wrote:
> > Yes, that might be clearer. I probably still would have to fix up
> > LIR_Assembler::comp_op() for more than just x86, since most would not
> work
> > for comparisons with T_ADDRESS constants.
> 
> I think this is just a problem for x86. We don't do anything like that
> on AArch64, and we always generate a full-width comparison:
> 
> void LIR_Assembler::comp_op(LIR_Condition condition, LIR_Opr opr1, LIR_Opr
> opr2, LIR_Op2* op) {
> ...
> if (opr2->is_constant()) {
> ...
> switch(opr2->type()) {
> ...
> case T_OBJECT:
> case T_ARRAY:
> jobject2reg(opr2->as_constant_ptr()->as_jobject(), rscratch1);
> __ cmpoop(reg1, rscratch1);
> 
> --
> Andrew Haley  (he/him)
> Java Platform Lead Engineer
> Red Hat UK Ltd. <https://www.redhat.com>
> https://keybase.io/andrewhaley
> EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671
> 
> 
Thanks for looking at this.

I'm not sure this is true though. The generic code compares a register with
an int constant:

// java.lang.Class::isPrimitive()
void LIRGenerator::do_isPrimitive(Intrinsic* x) {
.....
  __ cmp(lir_cond_notEqual, temp, LIR_OprFact::intConst(0));
.....
}

so would we not hit the T_INT path in LIR_Assembler::comp_op() and compare
with a 32bit value?

void LIR_Assembler::comp_op(LIR_Condition condition, LIR_Opr opr1, LIR_Opr
opr2, LIR_Op2* op) {
...
  } else if (opr1->is_single_cpu() || opr1->is_double_cpu()) {
    Register reg1 = as_reg(opr1);
    if (opr2->is_single_cpu()) {
...
    if (opr2->is_constant()) {
      bool is_32bit = false; // width of register operand
      jlong imm;

      switch(opr2->type()) {
      case T_INT:
        imm = opr2->as_constant_ptr()->as_jint();
        is_32bit = true;
        break;
...

My current fix makes this constant a metadata constant and adds comparisons
to T_METASPACE constants. I had to "dry-code" the aarch64 part, could you
please take a look whether this makes sense to you?

http://cr.openjdk.java.net/~stuefe/webrevs/8233019--c1-intrinsic-for-java.lang.class.isprimitive()-does-32bit-compare/webrev.00/webrev/


Thanks, Thomas


[prev in list] [next in list] [prev in thread] [next in thread]