[PATCH] D145238: [NVPTX] Expose LDU builtins

[Jakub Chlanda via Phabricator via cfe-commits]

jchlanda added inline comments.


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:18116
+    case NVPTX::BI__nvvm_ldu_h:
+      BuiltinName = "__nvvm_ldu_h";
+      break;
----------------
tra wrote:
> Can we use the standard `StringRef Name = getContext().BuiltinInfo.getName(BuiltinID);` to figure out the builtin name?
Ha, had a feeling it would exist, couldn't find it. Thanks.


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:18261-18263
+      std::string ErrMsg{HalfSupport.second};
+      CGM.Error(E->getExprLoc(),
+                ErrMsg.append(" requires native half type support.").c_str());
----------------
tra wrote:
> Nit: this would be a bit more readable:
> ```
> std::string BuiltinName{HalfSupport.second};
> CGM.Error(E->getExprLoc(),  BuiltinName + " requires native half type support.")` 
> ```
> You may also consider changing returned `BuiltinName` to be `std::string`, so you would not need an explicit temp var. 
Done the `std::string` return, thanks.


================
Comment at: llvm/test/CodeGen/NVPTX/ldu-ldg.ll:60
+define double @test_ldu_f64(ptr addrspace(1) %ptr) {
+; ldu.global.u64
+  %val = tail call double @llvm.nvvm.ldu.global.f.f64.p1(ptr addrspace(1) %ptr, i32 8)
----------------
tra wrote:
> Hmm. I wonder why we end up with `u64` here and not `b64`. Not that it matters in this case, but it is a discrepancy vs. `f16`.
That is copy/paste sloppiness on my part, sorry. I've updated the test to check generated PTX, not just the labels, and fixed the values.

It generates correct kinds of loads, based on the type, the only discrepancy is that it doesn't distinguish between signed and unsigned loads, always choosing the unsigned variant. I think that's by design, at [ISel](https://github.com/llvm/llvm-project/blob/main/llvm/lib/Target/NVPTX/NVPTXISelDAGToDAG.cpp#L1683) there is a check if the load needs to be extended and a correct `CVT` instruction will be added.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145238