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 loongarch 256-bit LASX SIMD optimization #1458

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jinboson
Copy link

With the bench_dwt utility, on loongarch64:

before changes: 30.990 s
with LASX optimization: 3.334 s


#ifdef __loongarch_asx
{
asm volatile
Copy link

Choose a reason for hiding this comment

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

Using "volatile" is generally a bad idea (unless there is a side-effect not explainable by the input/output/clobber). If it does not work w/o "volatile", you've likely missed some register input/output/clobber.

Copy link
Author

Choose a reason for hiding this comment

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

affect performance or something else ?

Copy link

Choose a reason for hiding this comment

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

Because "if it's a chicken, don't model it as a duck".

Copy link
Author

Choose a reason for hiding this comment

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

Please , provide helpful suggestions.

Copy link

Choose a reason for hiding this comment

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

Is it broken after if we remove volatile? If it's not broken, just remove volatile. Otherwise try to figure out why the compiler breaks it and fix the input/output/clobber list.

volatile means there is some unexplainable side effect, for example writing/reading some CSRs, invoking hardware memory barriers, etc. These just do not apply for a SIMD optimization.

Copy link
Author

Choose a reason for hiding this comment

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

just want to tell compiler not to optimize or modify the asm block.

Copy link

@xry111 xry111 Jul 7, 2023

Choose a reason for hiding this comment

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

just want to tell compiler not to optimize or modify the asm block.

It won't modify the asm block because the compiler does not parse asm at all.

The only possible optimizations are:

  1. Reorder the asm block (as a whole block: the compiler cannot insert something in the middle of the asm block because it does not know how to parse asm) with the instructions generated by the compiler itself.
  2. Remove the asm block completely (not "removing a part of the asm", again because the compiler does not know how to parse asm).

If the input/output/clobber lists are correct, 2 should be disabled, and 1 should be performed in a constrained way not to break the code.

Again volatile means "there is unexplainable side effect", not "don't modify the asm block".

Copy link

Choose a reason for hiding this comment

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

Well, and maybe

  1. Duplicating the asm block due to a loop unrolling etc. Again if the input/output/clobber lists are correct this should happen in a safe way.

asm volatile
(
"srli.w $t0, %[cblk], 4 \n\t"
"xvldrepl.w $xr0, %[step], 0 \n\t"
Copy link

Choose a reason for hiding this comment

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

Is there any reason not to use xvreplgr2vr.w here?

Copy link
Author

Choose a reason for hiding this comment

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

Nice catch, will update.

OPJ_INT32* ptr1 = datap + j * cblk_w;
OPJ_INT32* ptr2 = tiledp + j * (OPJ_SIZE_T)tile_w;
OPJ_UINT32 step_size = 0;
asm volatile
Copy link

Choose a reason for hiding this comment

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

Likewise, try to avoid volatile.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants