[PATCH] D125519: [TableGen][DirectX] Add tableGen backend to generate map from llvm intrinsic to DXIL operation.

[Justin Bogner via Phabricator via llvm-commits]

bogner accepted this revision.
bogner added inline comments.
This revision is now accepted and ready to land.


================
Comment at: llvm/lib/Target/DirectX/DXILOpLowering.cpp:67
   case OverloadKind::UserDefineType:
+  default:
     llvm_unreachable("invalid overload type for name");
----------------
Why do this? Since the switch statement covers all values of the enum there are nice warnings (at least if you build with clang) if someone adds another OverloadKind, and forgets to update here, whereas with the default statement that's not the case.

I guess some compilers might warn that not all paths return a value though if they think we can fall out of the switch. If you're just trying to handle that warning then it's probably best to move the llvm_unreachable + return statement out of the switch and just put break here, to get the best of both worlds.


================
Comment at: llvm/utils/TableGen/DXILEmitter.cpp:87
+      auto DefName = IntrinsicDef->getName();
+      // Remove the int_ from intrinsic name.
+      Intrinsic = DefName.substr(4);
----------------
Might be worth an assert that the intrinsic name starts with "int_" just in case someone makes a mistake in the td. OTOH I think we'll already error in that case because map_intrinsic won't find anything, so maybe it's fine.


================
Comment at: llvm/utils/TableGen/DXILEmitter.cpp:196
+  OS << "\n";
+  OS << "static const SmallDenseMap<Intrinsic::ID, DXIL::OpCode> LowerMap = "
+        "{\n";
----------------
Probably better done as a separate change, but is a SmallDenseMap really the best option here? We could probably just store this as an array of pairs and pre-sort it, then query it with std::lower_bound on the keys instead.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125519