craig.topper added inline comments.
================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:19975
+ llvm::Type *ResTy = ConvertType(E->getType());
+ ConstantInt *Mode = llvm::cast<llvm::ConstantInt>(Ops[1]);
+
----------------
Is `llvm::` needed on the `cast`? I think cast is imported into the clang
namespace.
================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:19975
+ llvm::Type *ResTy = ConvertType(E->getType());
+ ConstantInt *Mode = llvm::cast<llvm::ConstantInt>(Ops[1]);
+
----------------
craig.topper wrote:
> Is `llvm::` needed on the `cast`? I think cast is imported into the clang
> namespace.
You can drop llvm:: on ConstantInt in the cast since you didn't need it for the
`ConstantInt *Mode` part.
================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:19990
+ case RISCV::BI__builtin_riscv_ntl_store: {
+ ConstantInt *Mode = llvm::cast<llvm::ConstantInt>(Ops[2]);
+
----------------
Same comments as above
================
Comment at: clang/lib/Headers/riscv_ntlh.h:24
+
+#define __riscv_ntl_load(PTR, DOMAIN) __builtin_riscv_ntl_load(PTR, DOMAIN)
+#define __riscv_ntl_store(PTR, VAL, DOMAIN)
\
----------------
Need parentheses around PTR and DOMAIN in the expansion.
```
#define __riscv_ntl_load(PTR, DOMAIN) __builtin_riscv_ntl_load((PTR), (DOMAIN))
```
================
Comment at: clang/lib/Headers/riscv_ntlh.h:26
+#define __riscv_ntl_store(PTR, VAL, DOMAIN)
\
+ __builtin_riscv_ntl_store(PTR, VAL, DOMAIN)
+
----------------
Same here
================
Comment at: clang/test/CodeGen/RISCV/ntlh-intrinsics/riscv32-zihintntl.c:115
+ __riscv_ntl_store(&d1, 1.0, __RISCV_NTLH_ALL); // CHECK: store
double{{.*}}align 8, !nontemporal !7
+
+}
----------------
Do we need to test vectors?
================
Comment at: clang/test/CodeGen/RISCV/ntlh-intrinsics/riscv32-zihintntl.c:119
+
+// CHECK: !4 = !{i32 2}
+// CHECK: !5 = !{i32 3}
----------------
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.
================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoZihintntl.td:14
-let hasSideEffects = 0, mayLoad = 0, mayStore = 0, Size = 4 in {
- def PseudoNTLALL : Pseudo<(outs), (ins), [], "ntl.all">,
+let hasSideEffects = 0, mayLoad = 0, mayStore = 0, Size = 4, isCodeGenOnly = 1
in {
+ def PseudoNTLP1 : Pseudo<(outs), (ins), [], "ntl.p1">,
----------------
Doesn't Pseudo already set isCodeGenOnly=1?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D143364/new/
https://reviews.llvm.org/D143364
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
