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.

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

Add options to select the threshold for using rep movs in arraycopy #7400

Conversation

a7ehuo
Copy link
Contributor

@a7ehuo a7ehuo commented Jul 5, 2024

arraycopyRepMovsByteArrayThreshold

  • Byte array copy threshold for using REP MOVS instructions.
  • Only supports 32 or 64 bytes

arraycopyRepMovsCharArrayThreshold

  • Char array copy threshold for using REP MOVS instructions.
  • Only supports 32 or 64 bytes.

arraycopyRepMovsIntArrayThreshold

  • Int array copy threshold for using REP MOVS instructions.
  • Only supports 32, 64, or 128 bytes.

arraycopyRepMovsLongArrayThreshold

  • Long array copy threshold for using REP MOVS instructions.
  • Only supports 32, 64, or 128 bytes.

arraycopyRepMovsReferenceArrayThreshold

  • Reference array copy threshold for using REP MOVS instructions.
  • Only supports 32, 64, or 128 bytes.

@a7ehuo
Copy link
Contributor Author

a7ehuo commented Jul 5, 2024

@0xdaryl May I ask you to review this change? Thank you!

eclipse-openj9/openj9#19815 depends on this change

@vijaysun-omr @hzongaro fyi

int32_t _arraycopyRepMovsIntArrayThreshold; // Int array copy threshold for using REP MOVS instructions. Only supports 32, 64, or 128 bytes
int32_t _arraycopyRepMovsLongArrayThreshold; // Long array copy threshold for using REP MOVS instructions. Only supports 32, 64, or 128 bytes
int32_t _arraycopyRepMovsReferenceArrayThreshold; // Reference array copy threshold for using REP MOVS instructions. Only supports 32, 64, or 128 bytes

Copy link
Contributor

Choose a reason for hiding this comment

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

Because these thresholds are never validated against your stated limits, can you confirm no functional failures will occur if someone specifies a value other than the supported limits? If the answer is that functional failures could occur then we'll need some way of validating the range of these options.

Also, since these thresholds are initialized to 0 and only changed if someone uses the option, then isn't 0 also a supported threshold?

Copy link
Contributor Author

@a7ehuo a7ehuo Jul 6, 2024

Choose a reason for hiding this comment

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

CG always initializes the threshold to 32 first. It sets the value from arraycopyRepMovs*ArrayThreshold only when the value specified by arraycopyRepMovs*ArrayThreshold is the supported limit and greater than 32. Otherwise, the value is ignored. There should not be any functional failure if an unsupported limit is set on the command line.

Below is an example of how a threshold is set for long array in OMRTreeEvaluator.cpp:

   threshold = 32;

   switch (elementSize)
      {
      case 8:
         {
         ...
         int32_t newThreshold = cg->comp()->getOptions()->getArraycopyRepMovsLongArrayThreshold();
         if ((threshold < newThreshold) && ((newThreshold == 64) || (newThreshold == 128)))
            {
            // If the CPU doesn't support AVX512, reduce the threshold to 64 bytes
            threshold = ((newThreshold == 128) && !cg->comp()->target().cpu.supportsFeature(OMR_FEATURE_X86_AVX512F)) ? 64 : newThreshold;
            }
         }
         break;

since these thresholds are initialized to 0 and only changed if someone uses the option, then isn't 0 also a supported threshold?

The initial value as 0 in the Options should be fine. It is not used to initialize the value used in CG as mentioned on the above. The value from the command line options is only considered when it is greater than 32.

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand that threshold is initialized to constant 32, but the new options are used to initialize newThreshold in that code snippet. If the options aren't explicitly set then their value of 0 is very much a part of the equation to determine if threshold needs to be adjusted. That was the source of my question.

My reading of the code concludes that it should be fine though, and you verified that no functional issue will occur with an unsupported value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the clarification! Should the default value for _arraycopyRepMovs*ArrayThreshold in OMROptions be changed to 32 as default instead of 0?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that seems like a sensible default if that is the minimum each of those options could be.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in bcf5c45. Ready for another review. Thank you @0xdaryl

`arraycopyRepMovsByteArrayThreshold`
- Byte array copy threshold for using REP MOVS instructions.
- Only supports 32 or 64 bytes

`arraycopyRepMovsCharArrayThreshold`
- Char array copy threshold for using REP MOVS instructions.
- Only supports 32 or 64 bytes.

`arraycopyRepMovsIntArrayThreshold`
- Int array copy threshold for using REP MOVS instructions.
- Only supports 32, 64, or 128 bytes.

`arraycopyRepMovsLongArrayThreshold`
- Long array copy threshold for using REP MOVS instructions.
- Only supports 32, 64, or 128 bytes.

`arraycopyRepMovsReferenceArrayThreshold`
- Reference array copy threshold for using REP MOVS instructions.
- Only supports 32, 64, or 128 bytes.

Signed-off-by: Annabelle Huo <Annabelle.Huo@ibm.com>
@a7ehuo a7ehuo force-pushed the system-arraycopy-perf-add-option-threshold branch from e806552 to bcf5c45 Compare July 8, 2024 20:47
@0xdaryl
Copy link
Contributor

0xdaryl commented Jul 8, 2024

Jenkins build all

@0xdaryl
Copy link
Contributor

0xdaryl commented Jul 9, 2024

Jenkins build win,plinux

@0xdaryl 0xdaryl self-assigned this Jul 9, 2024
@0xdaryl 0xdaryl merged commit 330a128 into eclipse-omr:master Jul 9, 2024
16 of 17 checks passed
@a7ehuo a7ehuo deleted the system-arraycopy-perf-add-option-threshold branch October 28, 2024 14:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants