[PATCH] D143364: [RISCV] Support scalar/fix-length vector NTLH intrinsic with different domain

[Craig Topper via Phabricator via cfe-commits]

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