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

Simplify the intrinsics by blending non-policy / policy intrinsics into a single set #186

Merged
merged 9 commits into from
Jan 24, 2023
Merged

Conversation

eopXD
Copy link
Collaborator

@eopXD eopXD commented Jan 11, 2023

Intro

This works implements the simplification proposal proposed by Nick Knight @nick-knight [0]. The simplification proposal targets to blend the two existing sets of intrinsics, non-policy / policy intrinsics, into a single set, with default assumption of policy behavior as TAMA. The simplification reduces 25% of the overall out-going intrinsics.

This pull request can be split into 3 parts.

  • Change default TUMU intrinsics to TUMA
  • Change all intrinsics from TAMU to TAMA
  • Simplify intrinsics with policy suffix

The recommended way of reviewing this patch is to checkout the changes under each instruction mnemonics. Take vadd for example, you can checkout auto-generated/api-testing/vadd.c, auto-generated/overloaded-api-testing/vadd.c, auto-generated/policy_funcs/llvm-api-tests/vadd.c, auto-generated/policy_funcs/llvm-overloaded-tests/vadd.c to see the prototype changes.

[0] https://gist.github.com/nick-knight/6cb0b74b351a25323dfb1821d3a269b9

Overview

For intrinsics originally with default as TAMU

Before:

vint32m1_t vadd_vv_i32m1 (vint32m1_t op1, vint32m1_t op2, size_t vl);
vint32m1_t vadd_vv_i32m1_m (vbool32_t mask, vint32m1_t maskedoff, vint32m1_t op1, vint32m1_t op2, size_t vl);
vint32m1_t vadd_vv_i32m1_tu (vint32m1_t merge, vint32m1_t op1, vint32m1_t op2, size_t vl);
vint32m1_t vadd_vv_i32m1_ta (vint32m1_t op1, vint32m1_t op2, size_t vl);
vint32m1_t vadd_vv_i32m1_tuma (vbool32_t mask, vint32m1_t merge, vint32m1_t op1, vint32m1_t op2, size_t vl);
vint32m1_t vadd_vv_i32m1_tumu (vbool32_t mask, vint32m1_t merge, vint32m1_t op1, vint32m1_t op2, size_t vl);
vint32m1_t vadd_vv_i32m1_tama (vbool32_t mask, vint32m1_t op1, vint32m1_t op2, size_t vl);
vint32m1_t vadd_vv_i32m1_tamu (vbool32_t mask, vint32m1_t merge, vint32m1_t op1, vint32m1_t op2, size_t vl);

After:

vint32m1_t vadd_vv_i32m1 (vint32m1_t op1, vint32m1_t op2, size_t vl);
vint32m1_t vadd_vv_i32m1_m (vbool32_t mask, vint32m1_t op1, vint32m1_t op2, size_t vl);
vint32m1_t vadd_vv_i32m1_tu (vint32m1_t maskedoff, vint32m1_t op1, vint32m1_t op2, size_t vl);
vint32m1_t vadd_vv_i32m1_tum (vbool32_t mask, vint32m1_t maskedoff, vint32m1_t op1, vint32m1_t op2, size_t vl);
vint32m1_t vadd_vv_i32m1_tumu (vbool32_t mask, vint32m1_t maskedoff, vint32m1_t op1, vint32m1_t op2, size_t vl);
vint32m1_t vadd_vv_i32m1_mu (vbool32_t mask, vint32m1_t maskedoff, vint32m1_t op1, vint32m1_t op2, size_t vl);

For intrinsics originally with default as TUMU

Before:

vint32m1_t vredsum_vs_i32m1_i32m1 (vint32m1_t dest, vint32m1_t vector, vint32m1_t scalar, size_t vl);
vint32m1_t vredsum_vs_i32m1_i32m1_m (vbool32_t mask, vint32m1_t dest, vint32m1_t vector, vint32m1_t scalar, size_t vl);vint32m1_t vredsum_vs_i32m1_i32m1_tu (vint32m1_t merge, vint32m1_t vector, vint32m1_t scalar, size_t vl);
vint32m1_t vredsum_vs_i32m1_i32m1_ta (vint32m1_t vector, vint32m1_t scalar, size_t vl);
vint32m1_t vredsum_vs_i32m1_i32m1_tum (vbool32_t mask, vint32m1_t merge, vint32m1_t vector, vint32m1_t scalar, size_t vl);
vint32m1_t vredsum_vs_i32m1_i32m1_tam (vbool32_t mask, vint32m1_t vector, vint32m1_t scalar, size_t vl);

After:

vint32m1_t vredsum_vs_i32m1_i32m1 (vint32m1_t vector, vint32m1_t scalar, size_t vl);
vint32m1_t vredsum_vs_i32m1_i32m1_m (vbool32_t mask, vint32m1_t vector, vint32m1_t scalar, size_t vl);
vint32m1_t vredsum_vs_i32m1_i32m1_tu (vint32m1_t maskedoff, vint32m1_t vector, vint32m1_t scalar, size_t vl);
vint32m1_t vredsum_vs_i32m1_i32m1_tum (vbool32_t mask, vint32m1_t maskedoff, vint32m1_t vector, vint32m1_t scalar, size_t vl);

For the 1st ~ 3rd commit

The first part is needed before performing the second one, as the TUMU intrinsics are the special cases in the intrinsics and hope to be removed in the simplification [0].

Corresponding changes in LLVM is posted here:

For the 4th ~ 6th commit

The second part aligns all non-policy intrinsics to TAMA, which in general should be the default policy for the RVV instructions because a undisturbed policy will generally slow down an out-of-order core.

The masked-off operand for non-policy masked intrinsics is removed because the policy is set to TAMA.

Corresponding changes in LLVM is posted here:

For the 7th ~ 9th commit

The third part is the simplification intrinsics with a tail policy suffix.

  • Policy intrinsics of _ta, _tama is removed, as non-policy intrinsics is default to TAMA and we are blending the two sets of intrinsics (non-policy / policy) into one.
  • Policy intrinsics of _tamu is renamed to _mu
  • Policy intrinsics of _tuma is renamed to _tum

Corresponding changes in LLVM is posted here:

This works toward having all implicit intrinsics be set tail agnostic
and mask undisturbed.

Prototype of multiply-add and vslideup intrinsics are not affected by
this change since the vd participates the computation even unmasked
and set to tail-agnostic. They change in the semantics, which is the
policy bit (vta) setted.

Prototype of reduction, vslidedown, vcompress, vmv_s_x, and vfmv_s_f
are affected by this change. The destination parameter for their
unmasked non-policy (implicit) intrinsics is removed.

Upon change of prototype, vmv_s_x and vfmv_s_f is no longer able to
have an overloading version for its non-policy and tail agnostic
intrinsics.

Signed-off-by: eop Chen <eop.chen@sifive.com>
@eopXD eopXD changed the title Change default tail undisturbed intrinsics to tail agnostic Simplification for the intrinsics Jan 13, 2023
@eopXD
Copy link
Collaborator Author

eopXD commented Jan 13, 2023

Change: Having the simplification collected all under this PR because it makes more sense to have accept them as a whole.

Revised the cover letter of this pull request.

@eopXD eopXD changed the title Simplification for the intrinsics Simplify the intrinsics by blending non-policy / policy intrinsics into a single set Jan 13, 2023
Copy link
Collaborator

@nick-knight nick-knight left a comment

Choose a reason for hiding this comment

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

I believe this correctly implements my proposal. Good work!

My only complaint is the logical organization of the repository into intrinsics_funcs and policy_funcs: I would prefer to keep them all in the same directory. This is a minor complaint and I think it could be addressed in a follow-up PR.

@eopXD
Copy link
Collaborator Author

eopXD commented Jan 14, 2023

My only complaint is the logical organization of the repository into intrinsics_funcs and policy_funcs: I would prefer to keep them all in the same directory. This is a minor complaint and I think it could be addressed in a follow-up PR.

Yes, the hierarchy will be refined. Thank you for mentioning this.

@zakk0610
Copy link
Collaborator

Cool, I like this simplification version, thanks for making it happen!

nit: maybe we could have a tag before change the interface.

Due to change of default policy, the overloading version for non-policy
variants of `vid` and `viota` is no longer available.

Signed-off-by: eop Chen <eop.chen@sifive.com>
Signed-off-by: eop Chen <eop.chen@sifive.com>
@eopXD eopXD merged commit 4496dff into riscv-non-isa:master Jan 24, 2023
eopXD added a commit to llvm/llvm-project that referenced this pull request Jan 24, 2023
…ion intrinsics

The destination parameter is removed for non-policy unmasked intrinsics,
they are now tail agnostic. The non-policy masked intrinsics are tail
agnostic and masked undisturbed, just like normal instructions like
vadd.

This is the 1st commit of a patch-set that aims to remove the
`IsPrototypeDefaultTU` special case for the rvv-intrinsics.

The patch-set work towards the simplification proposal [0] of Nick
Knight, the plan is that after this patch-set, all non-policy
intrinsics will be aligned with default policy behavior of tail
agnostic and mask undisturbed. Then the next patch-set will aim
towards changing non-policy intrinsics to tail agnostic and mask
agnostic.

[0] https://gist.github.com/nick-knight/6cb0b74b351a25323dfb1821d3a269b9

Pull Request: riscv-non-isa/rvv-intrinsic-doc#186

Reviewed By: craig.topper

Differential Revision: https://reviews.llvm.org/D140895
eopXD added a commit to llvm/llvm-project that referenced this pull request Jan 24, 2023
This is the 1st commit of a patch-set that aims to change the default policy
for RVV intrinsics from TAMU to TAMA.

The patch-set work towards the simplification proposal [0] of Nick
Knight. After this patch-set, all intrinsics operates under a general
assumption that the policy behavior is agnostic.

You may find that most of the patches are NFC patches. They subtly remove
implicit assumptions that entangles the codebase, making the singular patch
that contains functional change clear and obvious.

In [2/15], The attribute `Policy::IsPolicyNone` may give the mis-perception
that an RVV intrinsic may operate without any policy. However this is not the
case because the policy CSR-s (`vta` and `vma`) always affect an RVV
instruction's behavior, except that some instructions have policy always set
as agnostic (e.g. instructions with a mask destination register is always
tail agnostic).

Next, to perform the change from TAMU to TAMA, we need to first remove
`Policy::PolicyType::Omit`. [4/15] ~ [12/15] removes it with NFC patches step
by step. Without the patches, directly applying [14/15] to the existing codebase
will not work because there will be complicated logics that are scattered in
places that is hard to maintain.

[1/15], [3/15] are not related to the main goal of this patch-set, they were
clean-up along the way as I was going through the codebase. [13/15] is a
clean-up that was an oversight in D141198.

Finally, [14/15] performs the functional change this patch-set aims for. The
default policy is changed from TAMU to TAMA. This affects the masked version of
the intrinsics without a policy suffix. The masked-off operand is removed. Due
to the removal, masked version of `vid` and `viota` intrinsics are no longer
available for overloading.

[15/15] is a final commit to set data members of `Policy` as constants. Through
the refactoring the class `Policy` is now correct-by-construction.

The next patch-set will be to remove redundant intrinsics with a policy suffix
`_ta` and `_tama` intrinsics are redundant and will be removed. Other policy
suffix will be renamed to adapt to the general assumption that policy is
generally agnostic.

[0] https://gist.github.com/nick-knight/6cb0b74b351a25323dfb1821d3a269b9

Pull Request: riscv-non-isa/rvv-intrinsic-doc#186

Reviewed By: craig.topper, kito-cheng

Differential Revision: https://reviews.llvm.org/D141573
CarlosAlbertoEnciso pushed a commit to SNSystems/llvm-debuginfo-analyzer that referenced this pull request Jan 25, 2023
…ion intrinsics

The destination parameter is removed for non-policy unmasked intrinsics,
they are now tail agnostic. The non-policy masked intrinsics are tail
agnostic and masked undisturbed, just like normal instructions like
vadd.

This is the 1st commit of a patch-set that aims to remove the
`IsPrototypeDefaultTU` special case for the rvv-intrinsics.

The patch-set work towards the simplification proposal [0] of Nick
Knight, the plan is that after this patch-set, all non-policy
intrinsics will be aligned with default policy behavior of tail
agnostic and mask undisturbed. Then the next patch-set will aim
towards changing non-policy intrinsics to tail agnostic and mask
agnostic.

[0] https://gist.github.com/nick-knight/6cb0b74b351a25323dfb1821d3a269b9

Pull Request: riscv-non-isa/rvv-intrinsic-doc#186

Reviewed By: craig.topper

Differential Revision: https://reviews.llvm.org/D140895
CarlosAlbertoEnciso pushed a commit to SNSystems/llvm-debuginfo-analyzer that referenced this pull request Jan 25, 2023
This is the 1st commit of a patch-set that aims to change the default policy
for RVV intrinsics from TAMU to TAMA.

The patch-set work towards the simplification proposal [0] of Nick
Knight. After this patch-set, all intrinsics operates under a general
assumption that the policy behavior is agnostic.

You may find that most of the patches are NFC patches. They subtly remove
implicit assumptions that entangles the codebase, making the singular patch
that contains functional change clear and obvious.

In [2/15], The attribute `Policy::IsPolicyNone` may give the mis-perception
that an RVV intrinsic may operate without any policy. However this is not the
case because the policy CSR-s (`vta` and `vma`) always affect an RVV
instruction's behavior, except that some instructions have policy always set
as agnostic (e.g. instructions with a mask destination register is always
tail agnostic).

Next, to perform the change from TAMU to TAMA, we need to first remove
`Policy::PolicyType::Omit`. [4/15] ~ [12/15] removes it with NFC patches step
by step. Without the patches, directly applying [14/15] to the existing codebase
will not work because there will be complicated logics that are scattered in
places that is hard to maintain.

[1/15], [3/15] are not related to the main goal of this patch-set, they were
clean-up along the way as I was going through the codebase. [13/15] is a
clean-up that was an oversight in D141198.

Finally, [14/15] performs the functional change this patch-set aims for. The
default policy is changed from TAMU to TAMA. This affects the masked version of
the intrinsics without a policy suffix. The masked-off operand is removed. Due
to the removal, masked version of `vid` and `viota` intrinsics are no longer
available for overloading.

[15/15] is a final commit to set data members of `Policy` as constants. Through
the refactoring the class `Policy` is now correct-by-construction.

The next patch-set will be to remove redundant intrinsics with a policy suffix
`_ta` and `_tama` intrinsics are redundant and will be removed. Other policy
suffix will be renamed to adapt to the general assumption that policy is
generally agnostic.

[0] https://gist.github.com/nick-knight/6cb0b74b351a25323dfb1821d3a269b9

Pull Request: riscv-non-isa/rvv-intrinsic-doc#186

Reviewed By: craig.topper, kito-cheng

Differential Revision: https://reviews.llvm.org/D141573
@eopXD eopXD deleted the remove-default-tu branch January 26, 2023 15:45
eopXD added a commit to llvm/llvm-project that referenced this pull request Jan 28, 2023
This patch works towards the simplification proposal [0] of Nick
Knight. After this patch, we have reduced the hierarchy of intrinsics
from two sets (non-policy and policy) into a single set, with a general
assumption that policy behavior is agnostic unless specified.

[0] https://gist.github.com/nick-knight/6cb0b74b351a25323dfb1821d3a269b9

Pull Request: riscv-non-isa/rvv-intrinsic-doc#186.

Depends on D141796.

Reviewed By: craig.topper, kito-cheng

Differential Revision: https://reviews.llvm.org/D142016
CarlosAlbertoEnciso pushed a commit to SNSystems/llvm-debuginfo-analyzer that referenced this pull request Jan 29, 2023
This patch works towards the simplification proposal [0] of Nick
Knight. After this patch, we have reduced the hierarchy of intrinsics
from two sets (non-policy and policy) into a single set, with a general
assumption that policy behavior is agnostic unless specified.

[0] https://gist.github.com/nick-knight/6cb0b74b351a25323dfb1821d3a269b9

Pull Request: riscv-non-isa/rvv-intrinsic-doc#186.

Depends on D141796.

Reviewed By: craig.topper, kito-cheng

Differential Revision: https://reviews.llvm.org/D142016
tru pushed a commit to llvm/llvm-project-release-prs that referenced this pull request Feb 5, 2023
This patch works towards the simplification proposal [0] of Nick
Knight. After this patch, we have reduced the hierarchy of intrinsics
from two sets (non-policy and policy) into a single set, with a general
assumption that policy behavior is agnostic unless specified.

[0] https://gist.github.com/nick-knight/6cb0b74b351a25323dfb1821d3a269b9

Pull Request: riscv-non-isa/rvv-intrinsic-doc#186.

Depends on D141796.

Reviewed By: craig.topper, kito-cheng

Differential Revision: https://reviews.llvm.org/D142016
veselypeta pushed a commit to veselypeta/cherillvm that referenced this pull request Jun 17, 2024
…ion intrinsics

The destination parameter is removed for non-policy unmasked intrinsics,
they are now tail agnostic. The non-policy masked intrinsics are tail
agnostic and masked undisturbed, just like normal instructions like
vadd.

This is the 1st commit of a patch-set that aims to remove the
`IsPrototypeDefaultTU` special case for the rvv-intrinsics.

The patch-set work towards the simplification proposal [0] of Nick
Knight, the plan is that after this patch-set, all non-policy
intrinsics will be aligned with default policy behavior of tail
agnostic and mask undisturbed. Then the next patch-set will aim
towards changing non-policy intrinsics to tail agnostic and mask
agnostic.

[0] https://gist.github.com/nick-knight/6cb0b74b351a25323dfb1821d3a269b9

Pull Request: riscv-non-isa/rvv-intrinsic-doc#186

Reviewed By: craig.topper

Differential Revision: https://reviews.llvm.org/D140895
veselypeta pushed a commit to veselypeta/cherillvm that referenced this pull request Jun 17, 2024
This is the 1st commit of a patch-set that aims to change the default policy
for RVV intrinsics from TAMU to TAMA.

The patch-set work towards the simplification proposal [0] of Nick
Knight. After this patch-set, all intrinsics operates under a general
assumption that the policy behavior is agnostic.

You may find that most of the patches are NFC patches. They subtly remove
implicit assumptions that entangles the codebase, making the singular patch
that contains functional change clear and obvious.

In [2/15], The attribute `Policy::IsPolicyNone` may give the mis-perception
that an RVV intrinsic may operate without any policy. However this is not the
case because the policy CSR-s (`vta` and `vma`) always affect an RVV
instruction's behavior, except that some instructions have policy always set
as agnostic (e.g. instructions with a mask destination register is always
tail agnostic).

Next, to perform the change from TAMU to TAMA, we need to first remove
`Policy::PolicyType::Omit`. [4/15] ~ [12/15] removes it with NFC patches step
by step. Without the patches, directly applying [14/15] to the existing codebase
will not work because there will be complicated logics that are scattered in
places that is hard to maintain.

[1/15], [3/15] are not related to the main goal of this patch-set, they were
clean-up along the way as I was going through the codebase. [13/15] is a
clean-up that was an oversight in D141198.

Finally, [14/15] performs the functional change this patch-set aims for. The
default policy is changed from TAMU to TAMA. This affects the masked version of
the intrinsics without a policy suffix. The masked-off operand is removed. Due
to the removal, masked version of `vid` and `viota` intrinsics are no longer
available for overloading.

[15/15] is a final commit to set data members of `Policy` as constants. Through
the refactoring the class `Policy` is now correct-by-construction.

The next patch-set will be to remove redundant intrinsics with a policy suffix
`_ta` and `_tama` intrinsics are redundant and will be removed. Other policy
suffix will be renamed to adapt to the general assumption that policy is
generally agnostic.

[0] https://gist.github.com/nick-knight/6cb0b74b351a25323dfb1821d3a269b9

Pull Request: riscv-non-isa/rvv-intrinsic-doc#186

Reviewed By: craig.topper, kito-cheng

Differential Revision: https://reviews.llvm.org/D141573
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.

None yet

3 participants