openj9 icon indicating copy to clipboard operation
openj9 copied to clipboard

Verify checkFieldAlignmentForAtomicLong() for inlining atomics

Open matthewhall2 opened this issue 1 year ago • 2 comments

The method checkFieldAlignmentForAtomicLong() checks for a multiple of 4:

bool
J9::Z::CodeGenerator::checkFieldAlignmentForAtomicLong()
   {
   TR_OpaqueClassBlock * classBlock = self()->comp()->fej9()->getSystemClassFromClassName("java/util/concurrent/atomic/AtomicLong", 38, true);

   // TR_J9SharedCacheVM::getSystemClassFromClassName can return 0 when it's impossible to relocate a J9Class later for AOT loads.
   if (!classBlock)
      return false;

   char* fieldName = "value";
   int32_t fieldNameLen = 5;
   char * fieldSig = "J";
   int32_t fieldSigLen = 1;
   int32_t intOrBoolOffset = self()->fe()->getObjectHeaderSizeInBytes() + self()->fej9()->getInstanceFieldOffset(classBlock, fieldName, fieldNameLen, fieldSig, fieldSigLen);
   return (intOrBoolOffset & 0x3) == 0;
   }

However, as this is checking for alignment for an 8 byte field is should probably be checking for a multiple of 8.

Additionally, there is no similar check happening for AtomicInteger in J9::Z::CodeGenerator::InlineDirecCall:

case TR::java_util_concurrent_atomic_AtomicBoolean_getAndSet:
      case TR::java_util_concurrent_atomic_AtomicInteger_getAndAdd:
      case TR::java_util_concurrent_atomic_AtomicInteger_getAndIncrement:
      case TR::java_util_concurrent_atomic_AtomicInteger_getAndDecrement:
      case TR::java_util_concurrent_atomic_AtomicInteger_getAndSet:
      case TR::java_util_concurrent_atomic_AtomicInteger_addAndGet:
      case TR::java_util_concurrent_atomic_AtomicInteger_incrementAndGet:
      case TR::java_util_concurrent_atomic_AtomicInteger_decrementAndGet:
         resultReg = TR::TreeEvaluator::inlineAtomicOps(node, cg, 4, methodSymbol);
         return true;
         break;

      case TR::java_util_concurrent_atomic_AtomicIntegerArray_getAndAdd:
      case TR::java_util_concurrent_atomic_AtomicIntegerArray_getAndIncrement:
      case TR::java_util_concurrent_atomic_AtomicIntegerArray_getAndDecrement:
      case TR::java_util_concurrent_atomic_AtomicIntegerArray_getAndSet:
      case TR::java_util_concurrent_atomic_AtomicIntegerArray_addAndGet:
      case TR::java_util_concurrent_atomic_AtomicIntegerArray_incrementAndGet:
      case TR::java_util_concurrent_atomic_AtomicIntegerArray_decrementAndGet:
         resultReg = TR::TreeEvaluator::inlineAtomicOps(node, cg, 4, methodSymbol, true);
         return true;
         break;

      case TR::java_util_concurrent_atomic_AtomicLong_addAndGet:
      case TR::java_util_concurrent_atomic_AtomicLong_getAndAdd:
      case TR::java_util_concurrent_atomic_AtomicLong_incrementAndGet:
      case TR::java_util_concurrent_atomic_AtomicLong_getAndIncrement:
      case TR::java_util_concurrent_atomic_AtomicLong_decrementAndGet:
      case TR::java_util_concurrent_atomic_AtomicLong_getAndDecrement:
         if (cg->checkFieldAlignmentForAtomicLong() && comp->target().cpu.isAtLeast(OMR_PROCESSOR_S390_Z196))
            {
            // TODO: I'm not sure we need the z196 restriction here given that the function already checks for z196 and
            // has a compare and swap fallback path
            resultReg = TR::TreeEvaluator::inlineAtomicOps(node, cg, 8, methodSymbol);
            return true;
            }
         break;

offset being verified also seems strange: objectheadersize + fieldoffset (instead of just field?)

Opening this issue to track investigation of the correct offset and if the verification is needed for AtomicInteger

@r30shah @keithc-ca

matthewhall2 avatar Sep 25 '24 18:09 matthewhall2