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

[Xiang Li via Phabricator via llvm-commits]

python3kgae added inline comments.


================
Comment at: llvm/lib/Target/DirectX/DXIL.td:36
+  int dxil_opid = 0;      // ID of DXIL operation
+  string dxil_class = ""; // name of the opcode class
+  string category = "";   // classification for this instruction
----------------
python3kgae wrote:
> beanz wrote:
> > nhaehnle wrote:
> > > 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;
> > > ```
> > Does DXIL Class need to be a string field here or can it just be the TableGen record class?
> dxil_class is to group dxil operations which can share same function signature. Dxil operations with same dxil class will share same function declaration to reduce number of function declarations added for dxil operations.
> 
> dxil_category is not that important. It is used to sort dxil operation and dxil class so similar operation/class are together in the enum.
> 
> I'll add the "marker" classes.
I'll change it to TableGen record class.


================
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
----------------
beanz wrote:
> nhaehnle wrote:
> > 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;
> > ```
> Does DXIL Class need to be a string field here or can it just be the TableGen record class?
dxil_class is to group dxil operations which can share same function signature. Dxil operations with same dxil class will share same function declaration to reduce number of function declarations added for dxil operations.

dxil_category is not that important. It is used to sort dxil operation and dxil class so similar operation/class are together in the enum.

I'll add the "marker" classes.


================
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.
----------------
nhaehnle wrote:
> 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>;
> ```
Thanks for the suggestion.
I'll add dxil_map_intrinsic.


================
Comment at: llvm/lib/Target/DirectX/DXIL.td:52
+                              // op. "" means no direct map intrinsic.
+  list<string> counters = []; // counters for this inst.
+
----------------
nhaehnle wrote:
> What's a "counter"?
Counter is used for stats like how many atomic/float/uint/int/... instructions are used in the program.

It will be added in later PR, but not a high priority right now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125435