Z: Inline any length 2d array allocation by ehsankianifar · Pull Request #22548 · eclipse-openj9/openj9

ehsankianifar · 2025-09-03T17:28:38Z

Adding instructions to inline allocation of two dimensional arrays with any length. Before this change, we only inline 2d arrays if at least one dimension length is zero.

ehsankianifar · 2025-09-05T19:34:12Z

@r30shah this PR is ready for review
the selected instructions are as flowing:

LA      GPR9, Auto[<auto slot -1>] 112(GPR5)
LHI     GPR0,0x2	    *Number of dimensions
LGFI    GPR10,8707328	*Class register
 
Label L0067:	# (Start of internal control flow)	
   *** In case length1 or length2 are zero, we use this as object size
LGHI    GPR7,0x10	 ; Load discontinuous array size.
LTGF    GPR8,#466 4(GPR9) ; Load 1st dim length.
BRC     VGNOP(0x8), Label L0069 ***If the first dim length is zero
BRC     BM(0x4), Label L0064    ***If dimension length is negative

LTGF    GPR12,#467 0(GPR9) ; Load 2st dim length.
BRC     VGNOP(0x8), Label L0070 ***If the second dim length is zero
   ***convert second dim length to aligned leaf array size
SLA     GPR12,3
BRC     B(0xd), Label L0064
AGHI    GPR12,0x8	
LGR     GPR7,GPR12	
Label L0070:	                ***If the second dim length is zero
LR      GPR12,GPR7	
MSGR    GPR7,GPR8      *** number of leaf arrays * size of leaf array
   ***convert first dim length to aligned array size
SLLG    GPR8,GPR8,2
AGHI    GPR8,0xf	
NILF    GPR8,-8	
AGR     GPR7,GPR8	 ; Total size in sizeReg.
BRC     B(0xd), Label L0064
Label L0069:	                ***If the first dim length is zero
LG      GPR6,#468 96(GPR13) ; Set result reg.
ALGR    GPR7,GPR6	
BRC     MASK12(0xb), Label L0064
CLG     GPR7,#469 104(GPR13)
BRC     BL(0x2), Label L0064  ***If heap top test fails

STG     GPR7,#470 96(GPR13) ; Heap top test pass. Update heap alloc.
LG      GPR3,#471 0(GPR9) ; Start first dim alloc
ST      GPR10,#472 0(GPR6) ***Store class
ST      GPR3,#473 4(GPR6)  ***Store length
CLIJ    GPR3,Label L0066,0,BH(mask=0x8), 	***If the first dim length is zero, we are done!
   ***Get ready to allocate leaf arrays
AGR     GPR8,GPR6	 ; dim1SizeReg points to the fist leaf array.
LG      GPR10,#474 92(GPR10)
L       GPR10,#475 0(GPR9)
RISBG   GPR12,GPR12,0,31,30	 ; Compressed dim2 size in higher 32bits.
RISBG   GPR3,GPR8,0,31,30	 ; Compressed dim2 reference in higher 32 bits.
LGHI    GPR7,0x8
   ***	Allocate leaf arrays in a loop
Label L0068:	
STG     GPR10,#476 0(GPR8) ; Start second dim alloc ***Store Class and length
STFH    GPR3,#477 0(GPR7,GPR6) ***Store compressed leaf address to first dim array
AGFR    GPR8,GPR12	***Next leaf array
AGHI    GPR7,0x4	***Next element of first dim array
AHHHR   GPR3,GPR12,GPR3	***Bump compressed reference address
BRCT    GPR3,Label L0068 ***Next leaf

BRC     BRU(0xf), Label L0066  ***Done! go to the end
Label L0064:	***Jump to slow path helper in OOL 
BRC     BRU(0xf), Outlined Label L0065
assocreg
Label L0066:	# (End of internal control flow)	
LGR     GPR7,GPR6	 ; Copy tmp result to final result.

I upload a full compilation logs with compressed and non compressed references here:
NoCompRefs.log
CompRefs.log

r30shah

I am going to load this PR and do in more depth review, some initial comments.

ehsankianifar · 2025-09-12T20:35:52Z

@r30shah I addressed the change requests. Please take another look when you have time. Thanks!

r30shah · 2025-09-16T15:07:11Z

      {
-      generateRRInstruction(cg, TR::InstOpCode::LGR, node, temp1Reg, targetReg);
-      generateRILInstruction(cg, TR::InstOpCode::AGFI, node, temp1Reg, zeroArraySizeAligned);
+      traceMsg(comp, "Inline allocation for multianewarray with element size:%d and leaf component size:%d", elementSize, componentSize);


Suggested change

traceMsg(comp, "Inline allocation for multianewarray with element size:%d and leaf component size:%d", elementSize, componentSize);

traceMsg(comp, "Inline allocation for multianewarray with element size: %d and leaf component size: %d", elementSize, componentSize);

r30shah · 2025-09-16T15:09:10Z

-#endif /* J9VM_GC_SPARSE_HEAP_ALLOCATION */
+   TR::LabelSymbol *controlFlowStartLabel = generateLabelSymbol(cg);
+   controlFlowStartLabel->setStartInternalControlFlow();
+   generateS390LabelInstruction(cg, TR::InstOpCode::label, node, controlFlowStartLabel);


Internal control flow starts after LTGF - not here.

r30shah · 2025-09-16T15:17:42Z

+   int32_t alignmentConstant = TR::Compiler->om.getObjectAlignmentInBytes();
+   bool useCompRefs = TR::Compiler->om.compressObjectReferences();
+   // Skip alignment if both header and elements are aligned.
+   bool alignFirstDim = !OMR::aligned(headerSize, alignmentConstant) || !OMR::aligned(elementSize, alignmentConstant);


First use if the alignFirstDim is at line 4915 - is there specific reason - it is defined here?
Also it would have been easier to read is you have something like following,

Suggested change

bool alignFirstDim = !OMR::aligned(headerSize, alignmentConstant) || !OMR::aligned(elementSize, alignmentConstant);

bool alignFirstDim = !(OMR::aligned(headerSize, alignmentConstant) && OMR::aligned(elementSize, alignmentConstant));

This is self-explanatory, you would not need the comment above.

r30shah · 2025-09-16T15:18:16Z

+   bool useCompRefs = TR::Compiler->om.compressObjectReferences();
+   // Skip alignment if both header and elements are aligned.
+   bool alignFirstDim = !OMR::aligned(headerSize, alignmentConstant) || !OMR::aligned(elementSize, alignmentConstant);
+   bool alignSecondDim = !OMR::aligned(headerSize, alignmentConstant) || !OMR::aligned(componentSize, alignmentConstant);


Similar comment as previous comment.

r30shah · 2025-09-16T15:29:43Z

+   // Skip alignment if both header and elements are aligned.
+   bool alignFirstDim = !OMR::aligned(headerSize, alignmentConstant) || !OMR::aligned(elementSize, alignmentConstant);
+   bool alignSecondDim = !OMR::aligned(headerSize, alignmentConstant) || !OMR::aligned(componentSize, alignmentConstant);
+   // If any dimension length is zero, resulting array have discontiguous array size


I think the comment is bit unclear - please elaborate a bit.

r30shah · 2025-09-16T16:06:45Z

+ * \details
+ * Allocate 2 dimensional multi new arrays of any type with inline instructions if the size is within the range of TLH.
+ * This helper needs the following conditions to work properly and the caller should check these before calling:
+ * 1. Must be a 64bit system.


I think having Asserts for these conditions makes more sense (Rather than having it in caller.)

I used simple ASSERT to emphasize the conditions. I do not expect to get invalid data since the caller check those. Please let me know if you prefer ASSERT_FATAL

I would prefer FATAL assert, it is not so much as current callers, this asserts are there to prevent in future using this generate helper function with the conditions with which current function is implemented

I converted to fatal assert

r30shah · 2025-09-16T16:08:18Z

+   // Need dimReg for helper.
+   TR::Register *dimReg = cg->evaluate(node->getSecondChild());
+   TR::Register *classReg = cg->gprClobberEvaluate(node->getThirdChild());
+   TR::Register *vmThreadReg = cg->getMethodMetaDataRealRegister();


Move declaration and definition of this register closer to where it is used (It is not needed for deps as well)

r30shah

I think I am 70% done with review - Only need to go through code that deals with the allocation / bookkeeping of second dimension arrays - but some minor comments / questions / suggestions based on the review done so far. Just cosmetic changes, no structural changes.

r30shah · 2025-09-19T18:37:20Z

 ///////////////////////////////////////////////////////////////////////////////////////
 // Generate code for multianewarray
-// Checks the number of dimensions. For 1 dimensional arrays call the helper, for >1 call
+// Checks the number of dimensions. For 1 dimensional arrays call the helper, for 2 call


I think a better comment here would need some more details. We are generating the inlined code for 2 dimensions. For all other cases we generate helper call, it is not only the 1 dimensional array.

Also out of curiosity - Why can't we generate inline code for 1 dimensional array, can we have multianewarray for 1-D array ?

since the number of dimensions is static, It would be very odd to receive a call to allocate one dimension array here! anewarray would gets called for those cases.

r30shah · 2025-09-19T18:44:12Z

   TR_ASSERT_FATAL(secondChild->getOpCodeValue() == TR::iconst, "dims of multianewarray must be iconst");
-
-   // Only generate inline code if nDims > 1
   uint32_t nDims = secondChild->get32bitIntegralValue();


Though, it was already there, suggestion,

Suggested change

uint32_t nDims = secondChild->get32bitIntegralValue();

uint32_t nDims = static_cast<uint32_t>(secondChild->get32bitIntegralValue());

r30shah · 2025-09-19T18:46:13Z

-static TR::Register * generateMultianewArrayWithInlineAllocators(TR::Node *node, TR::CodeGenerator *cg)
+/**
+ * \brief
+ * Inline allocation of two dimensional new array.


The name and brief do not match - I am ok to simply use multi dimensional array here. We can mention here that the helper currently only supports two dimensional array.

r30shah · 2025-09-19T18:56:07Z

+ * Inline allocation of two dimensional new array.
+ *
+ * \details
+ * Allocate 2 dimensional multi new arrays of any type with inline instructions if the size is within the range of TLH.


Name of the function and the comment (brief and details) is bit mismatching. I would convey the message in the details that this helper only supports multianew array with 2 dimensions. Also would spin the conditions you listed as current support statement (Mode definitive - compare to what you have right not (work properly))

r30shah · 2025-09-19T18:56:25Z

+ * This helper allocate any non negative size 2D array if it fits inside the TLH.
+ *
+ * \param node
+ * The node.


multianewarray node

r30shah · 2025-09-19T20:13:42Z

+
+   cursor = generateS390LabelInstruction(cg, TR::InstOpCode::label, node, zeroSecondDimLabel, cursor);
+   // If jumping from zero second dim match, set second dim size to discontiguous. Otherwise this wont change values.
+   cursor = generateRRInstruction(cg, TR::InstOpCode::LR, node, dim2SizeReg, sizeReg, cursor);


Why LR and not LGR - I assume the sizeReg here is the discontiguous size which you loaded at the beginning in the evaluator. Eventhough this would not cause any functional issue given the dim2SizeReg is loaded with LTGF - Please use LGR here.

the purpuse of LTGF is to zero the higher 32 bits of the 64bit register. Since we know that this size is less than 32bits and the higher 32bit is already zero, we can use LR here with is a 4byte instruction. LGR is a 8byte instruction.

This kind of assumptions are the one that could cause code to become difficult to understand / manage and may cause bug if something changes. What different does LR/LGR makes ? In your evaluator, operand is 64-bit - Why do we make assumption that it will never have upper half with some value so only load the 32-bit value.

I was trying to reduce code size by using LR but if you think LGR makes the code more readable, I'll use that.

It's more about the kind of assumptions - that later on leads to issues with upper half containing some stale values - you have written the code so you know about the assumption, I have reviewed the code so I know. But in future due to some issues if there is some change happens before this LR/LGR - this kind of change is so implicit that it is easy to forget. We would have the code in - and in rare highly intermittent case we would hit some unexpected result. This is the reason, we should try to work with the operand type as much as possible.

r30shah · 2025-09-19T21:04:04Z

+   // If jumping from zero second dim match, set second dim size to discontiguous. Otherwise this wont change values.
+   cursor = generateRRInstruction(cg, TR::InstOpCode::LR, node, dim2SizeReg, sizeReg, cursor);
+   // Total size = (second dim size * number of second dim arrays) + first dim size.
+   // Max possible value is uint32.max * int32.max which will not overflow.


Which operand is uint32? I think this comment is bit misleading - Please add more context with the assumption that you are taking - which is max number of bytes would not overflow 64-bit range.

r30shah · 2025-09-19T21:10:22Z

+   if (alignFirstDim)
+      cursor = generateRILInstruction(cg, TR::InstOpCode::NILF, node, dim1SizeReg, -alignmentConstant, cursor);
+
+   cursor = generateRREInstruction(cg, TR::InstOpCode::AGR, node, sizeReg, dim1SizeReg, cursor);


If we reach here, do we expect to be dealing with negative values ? What would be the consequences of using ALGR?

We already checked for negatives and max possible value is guaranteed to be a positive int64 value so both AGR and ALGR work fine here.

So can you use ALGR? that would guarantee to not cause fixed point overflow.

r30shah · 2025-09-19T21:11:52Z

+   cursor = generateRREInstruction(cg, TR::InstOpCode::AGR, node, sizeReg, dim1SizeReg, cursor);
+   iComment("Total size in sizeReg.");
+   // Only a positive number with no overflow is acceptable.
+   cursor = generateS390BranchInstruction(cg, TR::InstOpCode::BRC, TR::InstOpCode::COND_BRNP, node, inlineAllocFailLabel, cursor);


Given the zero length and negative length is already dealt - can we have 0/negative outcome ?

No it is not possible to have negative or zero. We only need to check for overflow here but since it has no cost, I used COND_BRNP to emphasize that the size after this branch is a positive integer!

Please adjust the condition based on the instruction change recommended in #22548 (comment)

I remove this branch since this adding can not overflow.

r30shah · 2025-09-19T21:14:18Z

+   // Update heap alloc.
+   cursor = generateRXInstruction(cg, TR::InstOpCode::STG, node, sizeReg, generateS390MemoryReference(vmThreadReg, heapAllocOffset, cg), cursor);
+   iComment("Heap top test pass. Update heap alloc.");
+   TR::Register *miscellaneousReg = cg->allocateRegister();


I think scratchReg is better suitable name.

ehsankianifar · 2025-09-23T14:24:37Z

Thanks @r30shah for your review. I addressed all the changes and the only thing left is the SLA vs SLLG instruction.
the problem is by using SLLG, we won't know if the shift result is larger than 32 bits. it could be large enough that overflow the multiply single instruction. I experimented with a few options like explicitly checking the higher 32bits of the size register after SLLG or using 128bit multiplication but it adds too much complexity. Maybe I can add more detailed explanation in the code and keep using SLA to mitigate the readability issue.

ehsankianifar · 2025-09-23T19:42:28Z

I get some failures in my jdk21 testing on Jenkins. I investigate to see if it is related to my changes.

r30shah · 2025-09-26T14:59:21Z

+   if (componentSize == 1)
+      {
+      // Make sure second dim length is not negative.
+      cursor = generateS390BranchInstruction(cg, TR::InstOpCode::BRC, TR::InstOpCode::COND_BL, node, inlineAllocFailLabel, cursor);


So my point about using the appropriate instruction for the data type was to ensure that we do not run into issues where some other change that happens causes incompatibility with your change down the road with such implicit assumptions like using SLA to only working with 32-bit signed length (# bytes) and assuming that we would never have the overflow with MSGR because different paths in your ICF and set of instructions you selected does not let the case which can have an overflow for long multiplication.

I do see the current implementation you have works on z196 or newer. We have instruction that is available in z14 or newer (Multiply Single) that you can use which can set the condition code that you can use. Second option is using multiply and check upper 64-bit with zero by using CGIJ instruction.

Bit hesitant with current logic you have though it simplifies the instruction you generated - it puts limitation on the size of the array you are allocating and that restriction is implicit - not something documented / written in the comment, and also some third person reading the code would not find easy to make enhancement / change and may introduce the bugs, but it you really think that this is the best sequence we can have and in field even with large TLH size, restricting a dimension length to the value you have due to very limited cases, I am ok with current change, and in that case, I would recommend you to put a detailed comments so that it becomes clear when someone looks/modifies the code.

ehsankianifar · 2025-09-26T15:19:57Z

   TR::Node::recreate(node, TR::acall);
-   TR::Register *targetReg2 = TR::TreeEvaluator::performCall(node, false, cg);
+   TR::Node *secondChild = node->getSecondChild();
+   if (secondChild->getOpCode().isLoadConst() || secondChild->getOpCodeValue() == TR::iRegLoad)


@r30shah No sure if this is necessary to convert to long. It doesn't cause any issue but I am wondering is having garbage data in the higher32 bits of the parameter register can cause any issues in any case.
Let me know if I should revert this change.

I do not think you need to do anything here. The child will be evaluated into parameter register by the parameter type that a helper call expects - Is there specific reason you need this change?

Thanks, A few tests started to fail and I saw wrong parameter value and thought maybe it is because of the higher half of the parameter register. Later I find that the register was evaluated after the branching to the helper therefore the value was wrong but I decided to make sure about this.

r30shah · 2025-09-26T15:23:21Z

   TR::Node::recreate(node, TR::acall);
-   TR::Register *targetReg2 = TR::TreeEvaluator::performCall(node, false, cg);
+   TR::Node *secondChild = node->getSecondChild();
+   if (secondChild->getOpCode().isLoadConst() || secondChild->getOpCodeValue() == TR::iRegLoad)


I do not think you need to do anything here. The child will be evaluated into parameter register by the parameter type that a helper call expects - Is there specific reason you need this change?

ehsankianifar · 2025-09-27T03:03:35Z

Sanity tests are passing.

r30shah · 2025-09-29T13:55:01Z

-   TR::Register *targetReg = cg->allocateRegister();
   TR::Instruction *cursor = NULL;
+   int32_t elementSize = TR::Compiler->om.sizeofReferenceField();
+   TR_ASSERT_FATAL(comp->target().is64Bit(), "multianewArrayEvaluator is only supported on 64-bit JVMs!");


It is getting bit crowded here. Can we add some spacing between ASSERTs? to make each ASSERT message and condition clear?

r30shah · 2025-09-29T13:58:17Z

-   generateS390LabelInstruction(cg, TR::InstOpCode::label, node, cFlowRegionStart);
+   /********************************************* Register setup *********************************************/
+   TR::Register *dimsPtrReg = cg->evaluate(node->getFirstChild());
+   TR::Register *classReg = cg->gprClobberEvaluate(node->getThirdChild());


Going to let this one pass - but this variable is used bit late.

r30shah · 2025-09-29T13:59:04Z

-   cursor = generateRXInstruction(cg, TR::InstOpCode::LGF, node, firstDimLenReg, generateS390MemoryReference(dimsPtrReg, 4, cg));
+   if (node->getSecondChild()->getReferenceCount() > 1)
+      {
+      // If dimensions child is referenced elsewhere, evaluate it before ICF.


I do not think this comment it needed. I believe this is pretty self explanatory (Given you are doing ref count check)

r30shah · 2025-09-29T14:11:00Z

+      // Load int32 second dim length to a 64 bit register.
+      cursor = generateRXInstruction(cg, TR::InstOpCode::LGF, node, dim2SizeReg, generateS390MemoryReference(dimsPtrReg, 0, cg), cursor);
+      iComment("Load 2st dim length.");
+      // To guarantee no overflow in size calculation, the `length * componentSize` should not be larger than int32.max. Additionaly, the


Here are the points I want to make clear here so that in future if we decide to go with using the other multiplication version or investigate why we fail allocating inline even when we set large enough TLH.

Dimension length type is Int32. For given component, if total number of bytes are larger than INT_MAX, there is a possibility that total bytes to allocate may overflow 64-bit range.

By using SLA instruction, even though we put a cap on allocating inline array way below the range that can cause 64-bit overflow, it will simplify the instructions you are generating and avoid the overflow check after multiplying dimension 1 bytes and dimension 2 bytes as well as at places which contributes to total bytes calculation.

So add the assumption that we are making with the dimension length.

Need to be clear on what we are choosing here - Choosing the simplicity / platform compatibility over the allocation limit (Which I do expect to be very rare)

Also this is source code that is going to be opened in the IDE or browser which will not be rendered for the mark down - using the special characters for markdown would not help at all - Looks bit odd.

Updated the comment

r30shah · 2025-09-29T14:13:48Z

+   cursor = generateS390BranchInstruction(cg, TR::InstOpCode::BRC, TR::InstOpCode::COND_MASK5, node, inlineAllocFailLabel, cursor);
+
+   int32_t headerSize= TR::Compiler->om.contiguousArrayHeaderSizeInBytes();
+   // `size = (size + alignmentConstant - 1) & -alignmentConstant` equation round up the size to a factor of alignmentConstant.


No need to put `` in the source code.

r30shah · 2025-09-29T14:58:22Z

+   // Fist dim class and length:
+   bool useCompRefs = TR::Compiler->om.compressObjectReferences();
+   cursor = generateRXInstruction(cg, useCompRefs ? TR::InstOpCode::ST : TR::InstOpCode::STG, node, classReg,
+      generateS390MemoryReference(resultReg, (int32_t)TR::Compiler->om.offsetOfObjectVftField(), cg), cursor);


Please use static_cast<int32_t>... - Also if other places in this evaluator does use C++ casting, please change that.

r30shah · 2025-09-29T15:00:17Z

+   cursor = generateRXInstruction(cg, TR::InstOpCode::LA, node, dim1SizeReg, generateS390MemoryReference(dim1SizeReg, resultReg, 0, cg), cursor);
+   iComment("dim1SizeReg points to the fist leaf array.");
+   // Load component class to class register. In case of comp refs, class is in high order!
+   cursor = generateRXInstruction(cg, TR::InstOpCode::LG, node, classReg, generateS390MemoryReference(classReg, offsetof(J9ArrayClass, componentType)+(useCompRefs ? 4 : 0), cg), cursor);


Suggested change

cursor = generateRXInstruction(cg, TR::InstOpCode::LG, node, classReg, generateS390MemoryReference(classReg, offsetof(J9ArrayClass, componentType)+(useCompRefs ? 4 : 0), cg), cursor);

cursor = generateRXInstruction(cg, TR::InstOpCode::LG, node, classReg, generateS390MemoryReference(classReg, offsetof(J9ArrayClass, componentType) + (useCompRefs ? 4 : 0), cg), cursor);

r30shah · 2025-09-29T15:01:07Z

+
+   cursor = generateRXInstruction(cg, TR::InstOpCode::LA, node, dim1SizeReg, generateS390MemoryReference(dim1SizeReg, resultReg, 0, cg), cursor);
+   iComment("dim1SizeReg points to the fist leaf array.");
+   // Load component class to class register. In case of comp refs, class is in high order!


What is high order ? Class is in the lower half of the double word right.

Thats correct, made the comment more clear.

r30shah · 2025-09-29T15:10:00Z

-         // dataAddr field. secondDimLenReg is expected to be NULL at this point.
-         cursor = generateRXInstruction(cg, TR::InstOpCode::STG, node, firstDimLenReg, generateS390MemoryReference(targetReg, fej9->getOffsetOfDiscontiguousDataAddrField(), cg));
-         iComment("Clear 1st dim dataAddr field.");
+         // Keep a compressed version of dim2 size in higher 32 bits.


This seems like an odd comment. It suggest compressed version of dim2 size not the compressed refs. Please update the comment to make it clear. Also having some layout information here would enhance the comment.

Updated the comment and added diagram

r30shah · 2025-09-29T15:14:19Z

-   generateRRInstruction(cg, TR::InstOpCode::LGR, node, temp2Reg, targetReg);
-   generateRRInstruction(cg, TR::InstOpCode::AGR, node, temp2Reg, temp1Reg);
-   if (cg->comp()->target().cpu.isAtLeast(OMR_PROCESSOR_S390_Z196))
+   // Load the address of the first element of the first dimension array in sizeReg.


Can you add some comment on the layout here to share how some of the initialization you are doing is done here?

Added comments to summarize the second dim allocation plan

r30shah

@ehsankianifar I think we are closer, some more changes.

r30shah · 2025-09-30T19:50:25Z

+   cursor = generateS390CompareAndBranchInstruction(cg, TR::InstOpCode::CL, node, scratchReg, 0, TR::InstOpCode::COND_BE, controlFlowEndLabel, false /* needsCC */, cursor);
+
+   cursor = generateRXInstruction(cg, TR::InstOpCode::LA, node, dim1SizeReg, generateS390MemoryReference(dim1SizeReg, resultReg, 0, cg), cursor);
+   iComment("dim1SizeReg points to the fist leaf array.");


r30shah · 2025-09-30T19:50:50Z

+
+   cursor = generateRXInstruction(cg, TR::InstOpCode::LA, node, dim1SizeReg, generateS390MemoryReference(dim1SizeReg, resultReg, 0, cg), cursor);
+   iComment("dim1SizeReg points to the fist leaf array.");
+   // Load component class to class register. In case of comp refs, the calss is 32bits long so using this register for leaf array length value as well.


Another typo, and please do not shorten compressedrefs to comp refs (We do not use that throughout our code and also comp is used throughout for the compiler object). Class pointer is is 32-bit wide (Opposite to long which is more suitable for array).

r30shah · 2025-09-30T20:15:52Z

+   // Load component class to class register. In case of comp refs, the calss is 32bits long so using this register for leaf array length value as well.
+   // ClassReg when non-comp refs: |     class      |
+   // ClassReg when comp refs:     | class | length |
+   cursor = generateRXInstruction(cg, TR::InstOpCode::LG, node, classReg, generateS390MemoryReference(classReg, offsetof(J9ArrayClass, componentType) + (useCompRefs ? 4 : 0), cg), cursor);


Hmm, I do get your intention to smartly combine the field to reduce number of operations we need to do. It does work now but is not easily extensible if for any reason object layout changes or in future we can not guarantee J9Class* to be 32-bit wide for compressed refs. So do not do that.

This is also bug/error prune - We are loading double word from middle of the field offset for componentType which will also load upper half of itable into the register which you are later updating by loading the length register.

So please revert to old way of updating the header.

It save us one store operation in the leaf allocation loop. It is very easy to change but should't we change it if the layout changes in future?

My point is that - If the layout changes or compressed ref address for J9Class is not guaranteed to fit 32-bits, kind of changes needed would be a lot, and it wouldn't be that simple.

You are also doing an extra operation to fit both class and length in one register, right?

Chenged to simple load/store

r30shah · 2025-09-30T20:18:31Z

+      cursor = generateRXInstruction(cg, TR::InstOpCode::L, node, classReg, generateS390MemoryReference(dimsPtrReg, 0, cg), cursor);
+      if (shiftAmount > 0)
+         {
+         // Keep a compressed dim2 size in higher 32 bits.


I do not think you addressed my previous comment, what is compressed dim2 size? Keep a compressed dim2 size in higher 32-bits has completely different meaning then For compressedref, dim2 size in higher 32-bits

I compressed the size by shifting to the right. So we have the actual size in lower 32bits and the shifted size in higher 32 bits. This way I can add this value to the compressed reference of the leaf array and get the compressed reference of the next leaf.

It is not termed as compressed size. You are shifting the size not compressing it.

r30shah · 2025-09-30T20:19:15Z

+         // dim2SizeReg when shift amount is 0:    |          unused            |    leaf array size    |
+         // dim2SizeReg when shift amount is > 0:  | compressed leaf array size |    leaf array size    |
+         cursor = generateRIEInstruction(cg, TR::InstOpCode::RISBG, node, dim2SizeReg, dim2SizeReg, 0, 31, 32-shiftAmount, cursor);
+         iComment("Compressed dim2 size in higher 32bits.");


Fix this comment. You are not compressing the dim2 size.

r30shah · 2025-09-30T20:19:42Z

+         // scratchReg when shift amount is 0:    |       leaf array length       |    number of leaf arrays    |
+         // scratchReg when shift amount is > 0:  | compressed leaf array address |    number of leaf arrays    |
+         cursor = generateRIEInstruction(cg, TR::InstOpCode::RISBG, node, scratchReg, dim1SizeReg, 0, 31, 32-shiftAmount, cursor);
+         iComment("Compressed dim2 reference in higher 32 bits.");


Fix this as well.

r30shah · 2025-09-30T20:20:29Z

+   // Start setting second dim:
+   TR::LabelSymbol *secondDimLabel = generateLabelSymbol(cg);
+   cursor = generateS390LabelInstruction(cg, TR::InstOpCode::label, node, secondDimLabel, cursor);
+   // Store the class field. In case of comp refs, store the class and length fields.


Please update the short for comp refs to compressedrefs throughout this evaluator.

r30shah · 2025-09-30T20:22:00Z

+   cursor = generateS390BranchInstruction(cg, TR::InstOpCode::BRC, TR::InstOpCode::COND_B, node, controlFlowEndLabel, cursor);
+   iComment("Allocation done!");
+
+   /********************************************* Unreachable zone *********************************************/


What do you mean by unreachable zone ?

Any code here will not be executed. I put it here to make it clear why the EXRL target is here.

Ok, so I do not like having two XC instruction to perform the clearing and having EXRL separate target. We should be able to generate better loop for that. In any case, this comment is not that clear. Can we rewrite the loop so that we do not need extra XC to be generated ?

r30shah · 2025-09-30T20:29:48Z

+   static bool disableBatchClear = feGetEnv("TR_DisableBatchClear") != NULL;
+   bool needInitialization = disableBatchClear && !node->canSkipZeroInitialization();
+   TR::LabelSymbol *memoryInitializationExrlTargetLabel = NULL;
+   if (needInitialization)


Hmm, Did you change the login for memclear ? Usually we would use XC and for residue, use the EXRL. Current implementation have two XC generated. We should simplify this code it does look messy.

r30shah

@ehsankianifar Some more suggestions, I think we are closer.

ehsankianifar · 2025-10-01T17:10:09Z

@ehsankianifar Some more suggestions, I think we are closer.

Thanks @r30shah,
The last push address the comments.

r30shah · 2025-10-01T17:35:26Z

Thanks @ehsankianifar - Just confirming before I go for final review, have you addressed comment in #22548 (comment) ?

ehsankianifar · 2025-10-02T15:56:19Z

@r30shah I changed the order of memory initialization instructions.

ehsankianifar · 2025-10-03T13:44:35Z

Thanks @ehsankianifar - Just confirming before I go for final review, have you addressed comment in #22548 (comment) ?

I changed the order of operations in my last commit so I clean 256 byte chunks first the clean the residual bytes. Also moved the EXRL target next to EXRL instruction.

r30shah

@ehsankianifar Looks OK to me, but will skim over once again to make sure if there is anything left to address. One comment I had - Please not it is comment, I do not expect you to make change, but just an answer. I am going to launch test.

r30shah · 2025-10-07T19:54:42Z


-   generateRRInstruction(cg, TR::InstOpCode::LGR, node, targetReg, targetReg2);
-   generateS390BranchInstruction(cg, TR::InstOpCode::BRC, TR::InstOpCode::COND_BRC, node, cFlowRegionDone);
+   generateRRInstruction(cg, TR::InstOpCode::LGR, node, resultReg, slowResultReg);


Asking, would it be possible to copy this to collectedResultReg ? so that we do not perform copy again in the mainline?

I was afraid using the collected reference register in the slow path may add it to the GC point but never tested to see what happens.

Ok. It would be good to understand if this can be issue or not. I do see other evaluators using the allocated collected reference register throughout the evaluator (Take a look at VMNew). Now it may be possible that these register is added to dependency list which is merged in OOL with the dependency list generated for the helper call and it is ok (It won't be spilled), in which case I think current way in multianewarray is fine, but something to look into in future.

r30shah · 2025-10-07T19:57:36Z

Can you squash commits?

Adding instructions to inline allocation of two dimensional arrays with any length. Before this change, we only inline 2d arrays if at least one dimension length is zero. signed-off-by: Ehsan Kiani Far <ehsan.kianifar@gmail.com>

ehsankianifar · 2025-10-07T20:08:44Z

Can you squash commits?

Thanks for the review. Comments squashed!

r30shah · 2025-10-07T20:43:24Z

jenkins test sanity zlinux jdk8,jdk21

r30shah

All tests have passed, and changes LGTM. I am merging this one.

multianewarray opcode instructions was updated in eclipse-openj9#22548 and the evaluation of the second child was changed to evaluate only if reference count is > 1. This caused the last multianewarray tree to have wrong dims register child and some failures. Remove the condition for evaluating children. Fix: eclipse-openj9#22757 signed-off-by: Ehsan Kiani Far <ehsan.kianifar@gmail.com>

ehsankianifar marked this pull request as draft September 3, 2025 17:28

ehsankianifar force-pushed the Z_inlineMultiANewArray branch 2 times, most recently from 1e70d33 to 46eb3fc Compare September 3, 2025 17:48

ehsankianifar mentioned this pull request Sep 4, 2025

ShareTwoDimensionArrayComponentSizeCalculationCrossPlatforms #22552

Merged

ehsankianifar force-pushed the Z_inlineMultiANewArray branch from 46eb3fc to 8550a97 Compare September 5, 2025 18:57

ehsankianifar marked this pull request as ready for review September 5, 2025 19:31

ehsankianifar force-pushed the Z_inlineMultiANewArray branch 2 times, most recently from 6863661 to 0e9c5ae Compare September 5, 2025 19:38

r30shah requested changes Sep 9, 2025

View reviewed changes

Comment thread runtime/compiler/z/codegen/J9TreeEvaluator.cpp Outdated

Comment thread runtime/compiler/z/codegen/J9TreeEvaluator.cpp

Comment thread runtime/compiler/z/codegen/J9TreeEvaluator.cpp Outdated

ehsankianifar force-pushed the Z_inlineMultiANewArray branch from 6f6c4f0 to 53f5d29 Compare September 12, 2025 20:26

r30shah reviewed Sep 16, 2025

View reviewed changes

ehsankianifar force-pushed the Z_inlineMultiANewArray branch 2 times, most recently from dc27cf3 to bc0f923 Compare September 17, 2025 15:34

r30shah reviewed Sep 19, 2025

View reviewed changes

ehsankianifar force-pushed the Z_inlineMultiANewArray branch 2 times, most recently from 5708636 to 56ef740 Compare September 23, 2025 14:05

ehsankianifar force-pushed the Z_inlineMultiANewArray branch from 56ef740 to 04f9af2 Compare September 25, 2025 17:02

ehsankianifar commented Sep 25, 2025

View reviewed changes

Comment thread runtime/compiler/z/codegen/J9TreeEvaluator.cpp

r30shah requested changes Sep 26, 2025

View reviewed changes

ehsankianifar commented Sep 26, 2025

View reviewed changes

r30shah requested changes Sep 26, 2025

View reviewed changes

ehsankianifar force-pushed the Z_inlineMultiANewArray branch from 2d3c0b3 to 70c4c81 Compare September 26, 2025 20:29

r30shah requested changes Sep 29, 2025

View reviewed changes

r30shah requested changes Sep 30, 2025

View reviewed changes

ehsankianifar force-pushed the Z_inlineMultiANewArray branch 3 times, most recently from ab0751c to 1c51da8 Compare October 1, 2025 17:06

0xdaryl added comp:jit arch:z labels Oct 1, 2025

ehsankianifar force-pushed the Z_inlineMultiANewArray branch 4 times, most recently from db78475 to c7f5f89 Compare October 2, 2025 21:02

ehsankianifar force-pushed the Z_inlineMultiANewArray branch from c7f5f89 to 91a89d2 Compare October 3, 2025 13:46

r30shah reviewed Oct 7, 2025

View reviewed changes

Z: Inline any length 2d array allocation

9452d2d

Adding instructions to inline allocation of two dimensional arrays with any length. Before this change, we only inline 2d arrays if at least one dimension length is zero. signed-off-by: Ehsan Kiani Far <ehsan.kianifar@gmail.com>

ehsankianifar force-pushed the Z_inlineMultiANewArray branch from 91a89d2 to 9452d2d Compare October 7, 2025 20:00

r30shah approved these changes Oct 8, 2025

View reviewed changes

r30shah merged commit 6267a60 into eclipse-openj9:master Oct 8, 2025
9 checks passed

ehsankianifar deleted the Z_inlineMultiANewArray branch October 8, 2025 15:19

r30shah mentioned this pull request Oct 9, 2025

abbs Segmentation error jitAMultiNewArray IOException: Bad file descriptor NegativeArraySizeException #22757

Closed

ehsankianifar mentioned this pull request Oct 10, 2025

Fix multiANewArray evaluator register dependency bug in IBM Z platform #22763

Merged

hzongaro mentioned this pull request Oct 22, 2025

X: implement general case multianewarrayEvaluator for 2D arrays #22562

Merged

	traceMsg(comp, "Inline allocation for multianewarray with element size:%d and leaf component size:%d", elementSize, componentSize);
	traceMsg(comp, "Inline allocation for multianewarray with element size: %d and leaf component size: %d", elementSize, componentSize);

	bool alignFirstDim = !OMR::aligned(headerSize, alignmentConstant) \|\| !OMR::aligned(elementSize, alignmentConstant);
	bool alignFirstDim = !(OMR::aligned(headerSize, alignmentConstant) && OMR::aligned(elementSize, alignmentConstant));

	uint32_t nDims = secondChild->get32bitIntegralValue();
	uint32_t nDims = static_cast<uint32_t>(secondChild->get32bitIntegralValue());

	cursor = generateRXInstruction(cg, TR::InstOpCode::LG, node, classReg, generateS390MemoryReference(classReg, offsetof(J9ArrayClass, componentType)+(useCompRefs ? 4 : 0), cg), cursor);
	cursor = generateRXInstruction(cg, TR::InstOpCode::LG, node, classReg, generateS390MemoryReference(classReg, offsetof(J9ArrayClass, componentType) + (useCompRefs ? 4 : 0), cg), cursor);

Uh oh!

Conversation

ehsankianifar commented Sep 3, 2025

Uh oh!

ehsankianifar commented Sep 5, 2025

Uh oh!

r30shah left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ehsankianifar commented Sep 12, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

r30shah left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ehsankianifar commented Sep 23, 2025

Uh oh!