Verify checkFieldAlignmentForAtomicLong() for inlining atomics

[matthewhall2]

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]

wrote a simple unit test

public class TestAlign {
        public static void main(String [] args) {
                AtomicLong atomL = new AtomicLong(0L);
                AtomicInteger atomI = new AtomicInteger(0);

                long b;
                long b = atomL.incrementAndGet();
                int c = atomI.incrementAndGet();
        }
}

and added printfs in checkFieldAlignmentForAtomicLong to verify the sizes.
I also added a method checkFieldAlignmentForAtomicInteger. Both chieldFieldAlignment... methods give the same results for Integer and Long atomics:
compressed: header size: 4, field offset: 4
non compressed: header size: 8, field offset: 8

meaning that the field offset for integers is also 8 for non-compressed refs.

Does this mean that the AtomicInteger methods need a similar check to AtomicLong @r30shah ?

[keithc-ca]

I think the intent of the check is to guard inlining methods of AtomicLong; the offset of the field needs to be a multiple of the alignment required by the processor. For int, I expect the alignment requirement is 4, but such fields are always 4-aligned so a check is not necessary. If Z196 supports 64-bit atomic operations at addresses that are a multiple of 4, then the alignment is also unnecessary for long, but if the field must be 8-aligned for 64-bit operations, then the mask in checkFieldAlignmentForAtomicLong() should be 7 (not 3).

[github-actions]

Issue Number: 20235
Status: Closed
Actual Components: comp:jit
Actual Assignees: No one :(
PR Assignees: matthewhall2