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

[Craig Topper via Phabricator via cfe-commits]

craig.topper added a comment.

There no backend tests in this patch.



================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:19625
     if ((ICEArguments & (1 << i)) == 0) {
       Ops.push_back(EmitScalarExpr(E->getArg(i)));
       continue;
----------------
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.


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:19827
+    llvm::Type *ResTy = ConvertType(E->getType());
+    ConstantInt *Mode = llvm::dyn_cast<llvm::ConstantInt>(Ops[1]);
+
----------------
Use `cast` and drop the assert


================
Comment at: clang/lib/Headers/riscv_ntlh.h:1
+enum {
+  __RISCV_NTLH_INNERMOST_PRIVATE = 2,
----------------
Need copyright header


================
Comment at: clang/lib/Headers/riscv_ntlh.h:1
+enum {
+  __RISCV_NTLH_INNERMOST_PRIVATE = 2,
----------------
craig.topper wrote:
> Need copyright header
Missing #include guard


================
Comment at: clang/lib/Headers/riscv_ntlh.h:2
+enum {
+  __RISCV_NTLH_INNERMOST_PRIVATE = 2,
+  __RISCV_NTLH_ALL_PRIVATE,
----------------
Need to guard all of this with `__riscv_zihintntl` being defined


================
Comment at: clang/lib/Headers/riscv_ntlh.h:8
+
+#define __rv_ntl_load(PTR, DOMAIN) __builtin_riscv_ntl_load(PTR, DOMAIN)
+#define __rv_ntl_store(PTR, VAL, DOMAIN)                                       \
----------------
This intrinsic name should start with `__riscv`


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:19396
         return false;
+      if (!TLI.areTwoSDNodeTargetMMOFlagsMergeable(*cast<LoadSDNode>(Val),
+                                                   *OtherLd))
----------------
Is there a test for this code?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D143364/new/

https://reviews.llvm.org/D143364