This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Support scalar/fix-length vector NTLH intrinsic with different domain
ClosedPublic

Authored by BeMg on Feb 5 2023, 8:38 PM.

Details

Summary

This commit implements the two NTLH intrinsic functions.

type __riscv_ntl_load (type *ptr, int domain);
void __riscv_ntl_store (type *ptr, type val, int domain);
enum {
  __RISCV_NTLH_INNERMOST_PRIVATE = 2,
  __RISCV_NTLH_ALL_PRIVATE,
  __RISCV_NTLH_INNERMOST_SHARED,
  __RISCV_NTLH_ALL
};

We encode the non-temporal domain into MachineMemOperand flags.

  1. Create the RISC-V built-in function with custom semantic checking.
  2. Assume the domain argument is a compile time constant,

and make it as LLVM IR metadata (nontemp_node).

  1. Encode domain value as two bits MachineMemOperand TargetMMOflag.
  2. According to MachineMemOperand TargetMMOflag, select corrsponding ntlh instruction.

Currently, it supports scalar type and fixed-length vector type.

Diff Detail

Event Timeline

BeMg created this revision.Feb 5 2023, 8:38 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 5 2023, 8:38 PM
BeMg requested review of this revision.Feb 5 2023, 8:38 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptFeb 5 2023, 8:38 PM
BeMg retitled this revision from [RISCV] Support scalar NTLH intrinsic with different domain to [RISCV] Support scalar/fix-length vector NTLH intrinsic with different domain.Feb 5 2023, 8:40 PM
BeMg edited the summary of this revision. (Show Details)
BeMg edited the summary of this revision. (Show Details)
BeMg updated this revision to Diff 509244.Mar 29 2023, 1:17 AM
BeMg edited the summary of this revision. (Show Details)

rebase

There no backend tests in this patch.

clang/lib/CodeGen/CGBuiltin.cpp
19771

Need to force ICEArguments for these intrinsics like we do for BI__builtin_rvv_vget_v and BI__builtin_rvv_vset_v otherwise we won't run the constant expression evaluator.

19973

Use cast and drop the assert

clang/lib/Headers/riscv_ntlh.h
2

Need copyright header

2

Missing #include guard

3

Need to guard all of this with __riscv_zihintntl being defined

9

This intrinsic name should start with __riscv

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
19442

Is there a test for this code?

BeMg updated this revision to Diff 512404.Apr 11 2023, 5:12 AM
  1. update ntlh intrinsic (rv -> riscv)
  2. Add testcase
  3. Add ICEArguments for ntl load/store
  4. update riscv_ntlh.h
BeMg edited the summary of this revision. (Show Details)Apr 11 2023, 5:12 AM
BeMg added a comment.Apr 11 2023, 5:25 AM
test_nontemporal_load_v16i8
test_nontemporal_load_v8i16

These two test cases in nontemporal.ll are affected by areTwoSDNodeTargetMMOFlagsMergeable.
Does we need extra testcases for areTwoSDNodeTargetMMOFlagsMergeable?

craig.topper added inline comments.Apr 11 2023, 12:14 PM
clang/lib/CodeGen/CGBuiltin.cpp
19973

Is llvm:: needed on the cast? I think cast is imported into the clang namespace.

19973

You can drop llvm:: on ConstantInt in the cast since you didn't need it for the ConstantInt *Mode part.

19988

Same comments as above

clang/lib/Headers/riscv_ntlh.h
25

Need parentheses around PTR and DOMAIN in the expansion.

#define __riscv_ntl_load(PTR, DOMAIN) __builtin_riscv_ntl_load((PTR), (DOMAIN))
27

Same here

clang/test/CodeGen/RISCV/ntlh-intrinsics/riscv32-zihintntl.c
116

Do we need to test vectors?

120

There are multiple places in the LangRef that say the nontemporal node can only have value of 1.

The optional !nontemporal metadata must reference a single metadata name <nontemp_node> corresponding to a metadata node with one i32 entry of value 1.

At the very least those need to be updated.

llvm/lib/Target/RISCV/RISCVInstrInfoZihintntl.td
15–21

Doesn't Pseudo already set isCodeGenOnly=1?

Code like llvm::combineMetadataForCSE assumes that the !nontemporal metadata can be copied from one node if both nodes have it. If the 2 nodes have different values we need a rule of which one to pick. We can't just pick arbitrarily.

What if we attached the domain as a separate RISC-V specific metadata and didn't change the nontemporal format? If optimizations drop the RISC-V specific part it would still be nontemporal, but get the default domain?

BeMg updated this revision to Diff 512708.Apr 12 2023, 2:23 AM
  1. Add extra metadata riscv-nontemporal-domain for NTLH intrinisc function domain level.
  2. Update testcase for new metadata and vector type
craig.topper added inline comments.Apr 12 2023, 10:39 AM
clang/test/CodeGen/RISCV/ntlh-intrinsics/riscv32-zihintntl.c
20

What about the rvv builtin types for scalable vectors?

llvm/lib/Target/RISCV/RISCVISelLowering.cpp
15766

Should we default Nontemporal level to 5 before looking at riscv-nontemporal-domain? Then only 2-5 are legal values?

llvm/lib/Target/RISCV/RISCVInstrInfo.cpp
2634

This uses "non-temporal" and the metadata uses "nontemporal". I think we should be consistent.

BeMg updated this revision to Diff 514149.Apr 17 2023, 3:11 AM
  1. riscv-non-temporal-domain -> riscv-nontemporal-domain
  2. NontemporalLevel default value is 5
BeMg added inline comments.Apr 17 2023, 3:38 AM
clang/test/CodeGen/RISCV/ntlh-intrinsics/riscv32-zihintntl.c
20

Should we support rvv builtin types for __riscv_ntl_load/store although we already have plan to support RVV builtin type in nontemporal by extra RVV intrinsic functions?

It need some extra code to support these function with scalable vector type.

craig.topper added inline comments.Apr 17 2023, 10:23 AM
clang/test/CodeGen/RISCV/ntlh-intrinsics/riscv32-zihintntl.c
20

Can you add tests to show it is not supported and gives an error message?

BeMg updated this revision to Diff 514604.Apr 18 2023, 4:45 AM
  1. Support RVV builtin type for __riscv_ntl_load/store
  2. add RVV builtin type testcase
This revision is now accepted and ready to land.Apr 18 2023, 4:12 PM
This revision was landed with ongoing or failed builds.Apr 24 2023, 8:20 PM
This revision was automatically updated to reflect the committed changes.
chapuni added inline comments.
clang/lib/Headers/CMakeLists.txt
360

Here

clang/test/CodeGen/RISCV/ntlh-intrinsics/riscv32-zihintntl.c
5

This is generated only if riscv is configured.

llvm/test/CodeGen/RISCV/nontemporal-scalable.ll