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

[Jakub Kuderski via Phabricator via llvm-commits]

kuhar added inline comments.


================
Comment at: llvm/lib/Target/DirectX/DXILOpLowering.cpp:31
+
+const char *DXILOpNamePrefix = "dx.op.";
+
----------------
nit: prefer `StringLiteral`


================
Comment at: llvm/lib/Target/DirectX/DXILOpLowering.cpp:33
+
+enum OverloadKind : uint16_t {
+  VOID = 1,
----------------
nit: prefer `enum class`


================
Comment at: llvm/lib/Target/DirectX/DXILOpLowering.cpp:83
+  case Type::IntegerTyID: {
+    IntegerType *pIT = dyn_cast<IntegerType>(pType);
+    unsigned Bits = pIT->getBitWidth();
----------------
Use `cast` if you know that the cast must succeed
also nit: don't prefix pointer variables with `p`


================
Comment at: llvm/lib/Target/DirectX/DXILOpLowering.cpp:94
+      return OverloadKind::I32;
+    case 64:
+      return OverloadKind::I64;
----------------
Can this be some other value than these cases, e.g., 128 or 63 bits? Should we have a default case with `llvm_unreachable`? 


================
Comment at: llvm/lib/Target/DirectX/DXILOpLowering.cpp:121-122
+    Ty->print(os);
+    os.flush();
+    return str;
+  }
----------------
nit: you can do `os.str()` which will flush for you.


================
Comment at: llvm/lib/Target/DirectX/DXILOpLowering.cpp:143
+const char *getOpCodeClassName(const OpCodeProperty &Prop) {
+  return OpCodeClassNames[static_cast<unsigned>(Prop.OpCodeClass)];
+}
----------------
Shouldn't we check that this array element can be accessed?


================
Comment at: llvm/lib/Target/DirectX/DXILOpLowering.cpp:149
+  if (Kind == OverloadKind::VOID) {
+    return (Twine(DXILOpNamePrefix) + Twine(getOpCodeClassName(Prop))).str();
+  } else {
----------------
I think you can do `Twine(x) + y`, without creating a second twine. If not, consider changing `char *` to `StringLiteral`


================
Comment at: llvm/lib/Target/DirectX/DXILOpLowering.cpp:157-160
+using OC = DXIL::OpCode;
+using OCC = DXIL::OpCodeClass;
+using OK = OverloadKind;
+using AK = Attribute::AttrKind;
----------------
Could we *not* introduce 2/3-letter-long acronyms like this? I think this hurts readability unless such types are very well known. 


================
Comment at: llvm/lib/Target/DirectX/DXILOpLowering.cpp:173
+  TmpProp.OpCode = DXILOp;
+  const OpCodeProperty *Prop = std::lower_bound(
+      &OpCodeProps[0],
----------------
nit: use `llvm::lower_bound`


================
Comment at: llvm/lib/Target/DirectX/DXILOpLowering.cpp:175
+      &OpCodeProps[0],
+      &OpCodeProps[sizeof(OpCodeProps) / sizeof(OpCodeProperty)], TmpProp,
+      [](const OpCodeProperty &a, const OpCodeProperty &b) {
----------------
I'm confused by this division. Can you pull this out into a local variable with a descriptive name and/or add a comment?


================
Comment at: llvm/lib/Target/DirectX/DXILOpLowering.cpp:187
+  // backend.
+  if (!(Prop->OverloadTys & Kind)) {
+    llvm_unreachable("invalid overload");
----------------
nit: can you compare to zero instead of implicitly converting to bool and negating?


================
Comment at: llvm/lib/Target/DirectX/DXILOpLowering.cpp:192
+  std::string FnName = constructOverloadName(Kind, OverloadTy, *Prop);
+  assert(M.getFunction(FnName) == nullptr && "function already exist");
+
----------------
nit: llvm prefers `!ptr` instead of `ptr == nullptr`. Also, please capitalize the assertion message.


================
Comment at: llvm/lib/Target/DirectX/DXILOpLowering.cpp:197
+
+  SmallVector<Type *, 4> ArgTypes;
+  // DXIL has i32 opcode as first arg.
----------------
Why exactly 4? If we don't know the expected size, prefer the automatic one with `SmallVector<Type *>`


================
Comment at: llvm/lib/Target/DirectX/DXILOpLowering.cpp:210-211
+  Value *DXILOpArg = B.getInt32(static_cast<unsigned>(DXILOp));
+  for (auto It = F.user_begin(); It != F.user_end();) {
+    User *U = *(It++);
+    CallInst *CI = dyn_cast<CallInst>(U);
----------------
nit: Could we `make_early_inc_range` instead?


================
Comment at: llvm/lib/Target/DirectX/DXILOpLowering.cpp:213
+    CallInst *CI = dyn_cast<CallInst>(U);
+    if (CI == nullptr)
+      continue;
----------------
nit: `!CI`


================
Comment at: llvm/lib/Target/DirectX/DXILOpLowering.cpp:215
+      continue;
+    IRBuilder<> B(CI);
+    SmallVector<Value *, 4> Args;
----------------
Why do we need a second builder?


================
Comment at: llvm/lib/Target/DirectX/DXILOpLowering.cpp:216
+    IRBuilder<> B(CI);
+    SmallVector<Value *, 4> Args;
+    Args.emplace_back(DXILOpArg);
----------------
same here


================
Comment at: llvm/lib/Target/DirectX/DXILOpLowering.cpp:229
+  bool Updated = false;
+  static DenseMap<Intrinsic::ID, DXIL::OpCode> LowerMap = {
+      {Intrinsic::sin, DXIL::OpCode::Sin}};
----------------
I think `SmallDenseMap` would work here


================
Comment at: llvm/lib/Target/DirectX/DXILOpLowering.cpp:231-232
+      {Intrinsic::sin, DXIL::OpCode::Sin}};
+  for (auto It = M.functions().begin(); It != M.functions().end();) {
+    Function &F = *(It++);
+    if (!F.isDeclaration())
----------------
also here


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124805