[PATCH] D125435: [TableGen][DirectX] Add tableGen backend to generate DXIL operation for DirectX backend.

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

nhaehnle added a comment.

+1 for creating this stuff using TableGen.

Part of it may be my unfamiliarity with the finer points of DXIL, part of it may be that not enough of what you're planning to do with it is visible, but some of the TableGen dialect here is quite unclear. What's a counter, what's a class, what's a category? What do those things mean and why should TableGen have to care?

It may be helpful if you augmented this patch with a bit more than just `sin`...



================
Comment at: llvm/lib/Target/DirectX/DXIL.td:36-37
+  int dxil_opid = 0;      // ID of DXIL operation
+  string dxil_class = ""; // name of the opcode class
+  string category = "";   // classification for this instruction
+  string doc = "";        // the documentation description of this instruction
----------------
It's hard to tell how you're going to use these variables, but I suspect that a more idiomatic solution would be to have `DxilClass` and `Category` classes here; they can be dummy "marker" classes, like so:
```
class dxil_class;
class dxil_category;

def unary : dxil_class;
def unary_float : dxil_category;
```


================
Comment at: llvm/lib/Target/DirectX/DXIL.td:50
+
+  string llvm_intrinsic = ""; // The intrinsic which map directly to this dxil
+                              // op. "" means no direct map intrinsic.
----------------
How about making this of type `Intrinsic` so you can have a more "type-safe" link between `dxil_inst` and the underlying intrinsics?

Okay, so that won't actually work verbatim if you sometimes want to not have an llvm_intrinsic. What you could do, and what aligns more with how TableGen is used in other backends, would be a second class. Basically, remove this variable here and add a class:
```
class dxil_map_intrinsic<Intrinsic llvm_intrinsic_> { Intrinsic llvm_intrinsic = llvm_intrinsic_; }
```
... which you can then use below as:
```
def Sin : dxil_op<"Sin", 13, "Unary", "returns sine(theta) for theta in radians.",
              "half;float;", "rn",
              [
                dxil_param<0, "$o", "", "operation result">,
                dxil_param<1, "i32", "opcode", "DXIL opcode">,
                dxil_param<2, "$o", "value", "input value">
              ]>,
             dxil_map_intrinsic<int_sin>;
```


================
Comment at: llvm/lib/Target/DirectX/DXIL.td:52
+                              // op. "" means no direct map intrinsic.
+  list<string> counters = []; // counters for this inst.
+
----------------
What's a "counter"?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125435