[PATCH] D124805: [DirectX backend] Add pass to lower llvm intrinsic into dxil op function.

[Nicolai Hähnle via Phabricator via llvm-commits]

nhaehnle added a comment.

Overall approach looks good to me, I just have a whole bunch of comments on the details.



================
Comment at: llvm/lib/Target/DirectX/DXILOpLowering.cpp:32-33
+
+DenseMap<Intrinsic::ID, DXIL::OpCode> LowerMap = {
+    {Intrinsic::sin, DXIL::OpCode::Sin}};
+
----------------
Non-trivial globals require an initializer that is executed when the libLLVM.so (or DLL) is loaded, which is a negative impact on a huge number of users of LLVM, including those not using the DirectX target.

I'd suggest making this a static function-local variable.


================
Comment at: llvm/lib/Target/DirectX/DXILOpLowering.cpp:162-165
+// FIXME: generate this table with tableGen.
+const OpCodeProperty OpCodeProps[] = {
+    {OC::Sin, "Sin", OCC::Unary, OK::FLOAT | OK::HALF, AK::ReadNone},
+};
----------------
Please do TableGen this. When you do, please consider replacing the inline `const char*` by an offset into a large string constant. This has two benefits:

1. It allows you to pack the struct more tightly, using a 32-bit or even 16-bit index instead of a 64-bit pointer.
2. It avoids the need for constant data relocations when loading libLLVM.so.

I'm okay with keeping the FIXME comment for now.


================
Comment at: llvm/lib/Target/DirectX/DXILOpLowering.cpp:173-178
+  auto Pos = std::lower_bound(
+      &OpCodeProps[0],
+      &OpCodeProps[sizeof(OpCodeProps) / sizeof(OpCodeProperty)], TmpProp,
+      [](const OpCodeProperty &a, const OpCodeProperty &b) {
+        return a.OpCode < b.OpCode;
+      });
----------------
I believe you can use `llvm::lower_bound(OpCodeProps, ...)` here.


================
Comment at: llvm/lib/Target/DirectX/DXILOpLowering.cpp:179-181
+  unsigned Index = std::distance(OpCodeProps, Pos);
+
+  const OpCodeProperty &Prop = OpCodeProps[Index];
----------------
Just `*Pos`? Or rename Pos to Prop?


================
Comment at: llvm/lib/Target/DirectX/DXILOpLowering.cpp:216-218
+    if (CI == nullptr) {
+      continue;
+    }
----------------
Nit: You don't need braces around single-line `if` bodies, and personally I think this is more readable without.


================
Comment at: llvm/lib/Target/DirectX/DXILOpLowering.cpp:233-235
+namespace {
+class DXILOpLowering : public ModulePass {
+public:
----------------
You may want to already make this code ready for use with the NewPassManager.


================
Comment at: llvm/test/CodeGen/DirectX/sin.ll:1
+; RUN: llc %s --filetype=asm -o - | FileCheck %s
+
----------------
Could this be an `opt` test?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124805