Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Intrinsic for AArch64 Arrays.fill(array, constant) #10207

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Intrinsic for AArch64 Arrays.fill(array, constant) #10207

wants to merge 2 commits into from

Conversation

JohnTortugo
Copy link

@JohnTortugo JohnTortugo commented Dec 2, 2024 •
edited
Loading

Please, consider this patch to intrinsify Arrays.fill(array, constant) for byte, short and int on AArch64 backend. The assembly code is a port of the HotSpot assembly for the same intrinsic. Thank you @gergo- for reviewing the draft version of this PR!

The below is a summary of some quick JMH tests that I ran on my dev machine.

Instr     Benchmark                       (size)   Mode  Cnt          Score          Error  Units
No        ArrayFillBenchmark.fill_bytes       16  thrpt    5  130,858,263.770 ±  7175243.892  ops/s
Yes       ArrayFillBenchmark.fill_bytes       16  thrpt    5  268,367,821.486 ±  2717945.345  ops/s
No        ArrayFillBenchmark.fill_bytes     1024  thrpt    5    7,284,374.553 ±   131421.427  ops/s
Yes       ArrayFillBenchmark.fill_bytes     1024  thrpt    5   94,448,799.400 ±  1096754.457  ops/s
No        ArrayFillBenchmark.fill_bytes     4096  thrpt    5    1,814,548.662 ±    33455.246  ops/s
Yes       ArrayFillBenchmark.fill_bytes     4096  thrpt    5   28,970,761.402 ±  2636159.892  ops/s

No        ArrayFillBenchmark.fill_shorts      16  thrpt    5  137,210,363.401 ±  6208453.450  ops/s
Yes       ArrayFillBenchmark.fill_shorts      16  thrpt    5  285,341,885.029 ± 11301099.208  ops/s
No        ArrayFillBenchmark.fill_shorts    1024  thrpt    5    7,178,718.150 ±   741170.216  ops/s
Yes       ArrayFillBenchmark.fill_shorts    1024  thrpt    5   53,996,849.866 ±  2353229.277  ops/s
No        ArrayFillBenchmark.fill_shorts    4096  thrpt    5    1,814,690.509 ±    13988.852  ops/s
Yes       ArrayFillBenchmark.fill_shorts    4096  thrpt    5   14,905,037.102 ±  1719260.337  ops/s

No        ArrayFillBenchmark.fill_ints        16  thrpt    5  132,083,594.942 ±  7995181.451  ops/s
Yes       ArrayFillBenchmark.fill_ints        16  thrpt    5  374,192,809.496 ±  5778630.471  ops/s
No        ArrayFillBenchmark.fill_ints      1024  thrpt    5    7,044,324.507 ±   130830.618  ops/s
Yes       ArrayFillBenchmark.fill_ints      1024  thrpt    5   28,677,308.237 ±  1515323.358  ops/s
No        ArrayFillBenchmark.fill_ints      4096  thrpt    5    1,648,798.029 ±     9113.592  ops/s
Yes       ArrayFillBenchmark.fill_ints      4096  thrpt    5    7,563,740.695 ±   627888.347  ops/s

@oracle-contributor-agreement oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Dec 2, 2024
@gergo- gergo- self-assigned this Dec 5, 2024
gergo-
gergo- reviewed Dec 5, 2024
View reviewed changes
Copy link
Member

@gergo- gergo- left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for keeping up this work! I left some high-level comments for now. I'll do some testing and a more thorough review of the assembly code soon.

compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/asm/aarch64/AArch64Assembler.java Show resolved Hide resolved
compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/core/aarch64/AArch64LIRGenerator.java Outdated Show resolved Hide resolved
compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/core/aarch64/AArch64LIRGenerator.java Outdated Show resolved Hide resolved
compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/hotspot/stubs/IntrinsicStubs.java Outdated Show resolved Hide resolved
.../src/jdk.graal.compiler/src/jdk/graal/compiler/replacements/StandardGraphBuilderPlugins.java Outdated Show resolved Hide resolved
.../src/jdk.graal.compiler/src/jdk/graal/compiler/replacements/StandardGraphBuilderPlugins.java Outdated Show resolved Hide resolved
.../src/jdk.graal.compiler/src/jdk/graal/compiler/replacements/StandardGraphBuilderPlugins.java Outdated Show resolved Hide resolved
.../src/jdk.graal.compiler/src/jdk/graal/compiler/replacements/StandardGraphBuilderPlugins.java Outdated Show resolved Hide resolved
@JohnTortugo
Copy link
Author

@gergo- - I believe I've addressed all your comments so far. Please, let me know if you have any other concerns.

gergo-
gergo- reviewed Dec 16, 2024
View reviewed changes
Copy link
Member

@gergo- gergo- left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added some requests for improved documentation, and the value to be filled should be a long throughout. I also started some checks on this PR, note that the style checks had some complaints. Please make sure mx checkstyle passes in the compiler suite.

This will soon be ready for internal testing.

compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/lir/aarch64/AArch64ArrayFillOp.java
/**
* Emits code which fills an array with a constant value. The assembly code in this intrinsic was
* based in the HotSpot's version of the same intrinsic.
*/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Constant value" is a bit misleading, since it doesn't have to be a compile-time constant. "Fixed value" maybe? Please also note that that only primitive values are supported, and that floating-point values are expected reinterpreted as integer bits.

compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/lir/aarch64/AArch64ArrayFillOp.java
* based in the HotSpot's version of the same intrinsic.
*/
@Opcode("ARRAYS_FILL")
public final class AArch64ArrayFillOp extends AArch64ComplexVectorOp {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a @SyncPort annotation referring to the exact piece of HotSpot code this is based on.

compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/lir/aarch64/AArch64ArrayFillOp.java

GraalError.guarantee(array.getPlatformKind() == AArch64Kind.QWORD, "pointer value expected");
GraalError.guarantee(length.getPlatformKind() == AArch64Kind.DWORD, "integer value expected in 'length'");
GraalError.guarantee(value.getPlatformKind() == AArch64Kind.DWORD, "integer value expected in 'value'.");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This guarantee cannot be correct anymore since you now support long values too.

compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/lir/aarch64/AArch64ArrayFillOp.java
int operationWidthB = 8;
int operationWidthH = 16;
int operationWidthW = 32;
int operationWidthDW = 64;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our normal style is to have these magic operation size numbers as explicit constants in the code. The difference between 32 and 64 is just much easier to see than the difference between operationWidthW and operationWidthDW.

Also note that you're defining DW as 64 bits, but that doesn't match our conventions: AArch64Kind.DWORD is 32 bits. So please drop these symbolic constants.

compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/replacements/nodes/ArrayFillNode.java

/**
* Fills in an array with a given constant value.
*/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest something like:

Fills an array by setting all elements to {@link #valueToFillWith}. The value must be a primitive integer, and its bit size must match the array's {@link #elementKind}. Filling of floating-point arrays is supported by passing a floating-point value reinterpreted as integer bits.

compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/replacements/nodes/ArrayFillNode.java

protected JavaKind elementKind;

@Input protected ValueNode arrayBase;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest naming this just array and adding a comment like /** Address of the array object on the Java heap. */. The term "base" could be misleading, since we often use "base" for the address of the first array element, as opposed to the address of the whole object.

compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/replacements/nodes/ArrayFillNode.java
@Input protected ValueNode valueToFillWith;

public ArrayFillNode(ValueNode arrayBase, ValueNode offsetToFirstElement, ValueNode arrayLength, ValueNode valueToFillWith, @ConstantNodeParameter JavaKind elementKind) {
super(TYPE, StampFactory.forVoid(), null, LocationIdentity.any());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please unify with the other constructor:

Suggested change
super(TYPE, StampFactory.forVoid(), null, LocationIdentity.any());
this(array, offsetToFirstElement, arrayLength, valueToFillWith, elementKind, null);

Though I'm not even sure you need to pass the runtimeCheckedCPUFeatures everywhere.

compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/replacements/nodes/ArrayFillNode.java

public ArrayFillNode(ValueNode arrayBase, ValueNode offsetToFirstElement, ValueNode arrayLength, ValueNode valueToFillWith, @ConstantNodeParameter JavaKind elementKind,
@ConstantNodeParameter EnumSet<?> runtimeCheckedCPUFeatures) {
super(TYPE, StampFactory.forVoid(), runtimeCheckedCPUFeatures, LocationIdentity.any());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use NamedLocationIdentity.arrayLocation(elementKind) instead of any.

compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/replacements/nodes/ArrayFillNode.java
this.offsetToFirstElement = offsetToFirstElement;
this.arrayLength = arrayLength;
this.valueToFillWith = valueToFillWith;
this.elementKind = elementKind;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add guarantees for:

  • valueToFillWith.stamp(NodeView.DEFAULT) instanceof IntegerStamp
  • elementKind.isPrimitive()
  • PrimitiveStamp.getBits(valueToFillWith.stamp(NodeView.DEFAULT)) == elementKind.getBitCount()

compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/replacements/nodes/ArrayFillNode.java
@GenerateStub(name = "longArrayFill", parameters = {"Long"})
@GenerateStub(name = "floatArrayFill", parameters = {"Float"})
@GenerateStub(name = "doubleArrayFill", parameters = {"Double"})
public static native void fill(Pointer array, long offset, int length, int value,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

int value cannot be correct if you support long and double fills.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gergo- gergo- gergo- left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@gergo- gergo-

Labels
OCA Verified All contributors have signed the Oracle Contributor Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@JohnTortugo @gergo-