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

[Justin Bogner via Phabricator via llvm-commits]

bogner accepted this revision.
bogner added a comment.
This revision is now accepted and ready to land.

LGTM with a few minor nitpicks / clean ups



================
Comment at: llvm/lib/Target/DirectX/DXIL.td:31
+
+// "The parameter description for a DXIL instruction"
+class dxil_param<int _pos, string type, string _name, string _doc,
----------------
Why is this comment quoted?


================
Comment at: llvm/test/CodeGen/DirectX/umax.ll:5
+; CHECK:call i32 @dx.op.binary.i32(i32 39, i32 %{{.*}}, i32 %{{.*}})
+; CHECK:call i64 @dx.op.binary.i64(i32 39, i64 %{{.*}}, i64 %{{.*}})
+
----------------
Probably better to put these check lines in the function bodies near what they're checking, and add `CHECK-LABEL` for the function names to anchor them. It doesn't matter much when the test is small like this, but it's good practice and makes it easier to add more test cases in the future


================
Comment at: llvm/test/CodeGen/DirectX/umax.ll:11
+; Function Attrs: noinline nounwind optnone
+define noundef i32 @_Z3foof(i32 noundef %a, i32 noundef %b) #0 {
+entry:
----------------
Better to name functions after what they're testing (test_umax_i32 or something), for similar reasons as my comment about CHECKs - it makes it easier / more obvious when adding more tests to a test file.


================
Comment at: llvm/test/CodeGen/DirectX/umax.ll:34
+!0 = !{i32 1, !"wchar_size", i32 4}
+!1 = !{!"clang version 15.0.0 (https://github.com/llvm/llvm-project.git 73417c517644db5c419c85c0b3cb6750172fcab5)"}
----------------
You can delete the `llvm.module.flags` and `llvm.ident` metadata, it won't affect this test


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125435