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

zihintntl Intrinsic function proposal #47

Merged
merged 3 commits into from
Jul 25, 2023
Merged

zihintntl Intrinsic function proposal #47

merged 3 commits into from
Jul 25, 2023

Conversation

BeMg
Copy link
Contributor

@BeMg BeMg commented Jul 19, 2023

This is a proposal for zihintntl intrinsic function.

@asb
Copy link
Contributor

asb commented Jul 19, 2023 •
edited
Loading

For further context, I understand this intends to match what Clang currently provides (which is gated on enabling experimental extension support right now - awaiting the intrinsics being standardised).

@kito-cheng
Copy link
Collaborator

For more context: this also match what @cmuellner proposed #30 (comment) before

@kito-cheng kito-cheng mentioned this pull request Jul 19, 2023
Open
@kito-cheng
Copy link
Collaborator

c.c. @aswaterman

@kito-cheng
Copy link
Collaborator

This is LGTM, and although this is not implemented on GCC, but I think this is implementable and we have enough time frame to implement that before next release (which expected released at 2024/04).

@aswaterman
Copy link
Contributor

aswaterman commented Jul 19, 2023 •
edited
Loading

LGTM. The only comment I'd make is that we might consider adding overloaded versions that omit the domain argument, implying ALL. But I don't know offhand whether overloaded intrinsics are valid in C (as opposed to C++), and maybe this "easy-mode" will prove controversial for other reasons--so if there's disagreement, I don't want to block forward progress.

@nick-knight
Copy link

But I don't know offhand whether overloaded intrinsics are valid in C (as opposed to C++),

Both GCC's and Clang's C-language dialects support a limited form of C++ function overloading. We use this already in the vector intrinsics ("overloaded API"). Since these are the only compilers we require to prove concepts, this seems actionable here as well.

@BeMg
Copy link
Contributor Author

BeMg commented Jul 20, 2023

LGTM. The only comment I'd make is that we might consider adding overloaded versions that omit the domain argument, implying ALL. But I don't know offhand whether overloaded intrinsics are valid in C (as opposed to C++), and maybe this "easy-mode" will prove controversial for other reasons--so if there's disagreement, I don't want to block forward progress.

The version that omit the domain argument I think the clang exists the similar built-in function __builtin_nontemporal_load/store [1].

[1] https://clang.llvm.org/docs/LanguageExtensions.html#non-temporal-load-store-builtins

@aswaterman
Copy link
Contributor

aswaterman commented Jul 21, 2023 •
edited
Loading

@BeMg OK. Can we document that API here so that GCC and LLVM will match? Or is the problem that we can't make it part of this API because it does not include the __riscv prefix?

If the answer is that we can't include __builtin_nontemporal_load/store in this API, then I'll again recommend we add the overloaded variants that imply domain=ALL.

@BeMg
Copy link
Contributor Author

BeMg commented Jul 21, 2023

I'm not sure whether to add the __builtin_nontemporal_load/store to proposal. Besides it doesn't not including the __riscv prefix, GCC seem doesn't has these built-in functions now.

cc @kito-cheng

The overloaded version look like more consistent, so I add it into proposal.

It could be implemented by some macros.

#define __riscv_ntl_load_with_domain(PTR, DOMAIN) __builtin_riscv_ntl_load((PTR), (DOMAIN))
#define __riscv_ntl_load_without_domain(PTR) __builtin_riscv_ntl_load((PTR), __RISCV_NTLH_ALL)

#define SELECT_NTL_LOAD(_1,_2,NAME,...) NAME
#define __riscv_ntl_load(...) SELECT_NTL_LOAD(__VA_ARGS__, __riscv_ntl_load_with_domain, __riscv_ntl_load_without_domain)(__VA_ARGS__)

kito-cheng
kito-cheng approved these changes Jul 24, 2023
View reviewed changes
Copy link
Collaborator

@kito-cheng kito-cheng left a comment

Choose a reason for hiding this comment

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

LGTM, seems like the overloading trick is work both on GCC and clang/LLVM, and I will try to find some resource to implement GCC part.

I would like to get explicitly ack from @aswaterman and @asb before merge.

@asb
Copy link
Contributor

asb commented Jul 24, 2023

LGTM, seems like the overloading trick is work both on GCC and clang/LLVM, and I will try to find some resource to implement GCC part.

I would like to get explicitly ack from @aswaterman and @asb before merge.

LGTM.

@aswaterman
Copy link
Contributor

OK with me, too.

@BeMg
Copy link
Contributor Author

BeMg commented Jul 25, 2023

I will implement the overloaded version in LLVM.

@kito-cheng
Copy link
Collaborator

Thanks for everyone, I gonna merge this :)

@kito-cheng kito-cheng merged commit 01b90ea into riscv-non-isa:master Jul 25, 2023
topperc
topperc reviewed Jul 25, 2023
View reviewed changes
riscv-c-api.md
@@ -235,6 +235,51 @@ long __riscv_clmul (long a, long b); // clmul rd, rs1, rs2
vint8m1_t __riscv_vadd_vv_i8m1(vint8m1_t vs2, vint8m1_t vs1, size_t vl); // vadd.vv vd, vs2, vs1
```

### NTLH Intrisics

The RISC-V zihintntl extension provides the RISC-V specific intrinsic functions for generating non-temporal memory accesses. These intrinsic functions provide the domain parameter to specify the behavior of memory accesses.
Copy link
Contributor

Choose a reason for hiding this comment

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

zihintntl -> Zihintntl

BeMg added a commit to llvm/llvm-project that referenced this pull request Aug 4, 2023
@BeMg
[RISCV] Support overloaded version ntlh intrinsic function
2df05cd 
Here is the proposal riscv-non-isa/riscv-c-api-doc#47.

The version that omit the domain argument imply domain=__RISCV_NTLH_ALL.

```
type __riscv_ntl_load (type *ptr);
void __riscv_ntl_store (type *ptr, type val);
```

Reviewed By: kito-cheng, craig.topper

Differential Revision: https://reviews.llvm.org/D156221
razmser pushed a commit to razmser/llvm-project that referenced this pull request Sep 8, 2023
@BeMg @razmser
[RISCV] Support overloaded version ntlh intrinsic function
d87b415 
Here is the proposal riscv-non-isa/riscv-c-api-doc#47.

The version that omit the domain argument imply domain=__RISCV_NTLH_ALL.

```
type __riscv_ntl_load (type *ptr);
void __riscv_ntl_store (type *ptr, type val);
```

Reviewed By: kito-cheng, craig.topper

Differential Revision: https://reviews.llvm.org/D156221
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@topperc topperc topperc left review comments

@kito-cheng kito-cheng kito-cheng approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@BeMg @asb @kito-cheng @aswaterman @nick-knight @topperc