This is an archive of the discontinued LLVM Phabricator instance.

[TableGen][DirectX] Add tableGen backend to generate DXIL operation for DirectX backend.
ClosedPublic

Authored by python3kgae on May 11 2022, 4:59 PM.

Details

Summary

A new tableGen backend gen-dxil-enum is added to generate enum for DXIL operation and operation class.

A new file "DXILConstants.inc" will be generated when build DirectX target which include the enums.

More tableGen backends will be added to replace manually written table in DirectX backend.
The unused fields in dxil_inst will be used in future PR.

Diff Detail

Event Timeline

python3kgae created this revision.May 11 2022, 4:59 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 11 2022, 4:59 PM
python3kgae requested review of this revision.May 11 2022, 4:59 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 11 2022, 4:59 PM

+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...

llvm/lib/Target/DirectX/DXIL.td
37–38

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;
51

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>;
53

What's a "counter"?

beanz added inline comments.May 16 2022, 5:29 PM
llvm/lib/Target/DirectX/DXIL.td
37

Does DXIL Class need to be a string field here or can it just be the TableGen record class?

python3kgae added inline comments.May 16 2022, 11:49 PM
llvm/lib/Target/DirectX/DXIL.td
37

I'll change it to TableGen record class.

37–38

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.

51

Thanks for the suggestion.
I'll add dxil_map_intrinsic.

53

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.

Update for comments.

Rename to prepare for make all tableGen work for DXIL operation in one tableGen backend.

python3kgae marked 4 inline comments as done.Jun 13 2022, 9:38 AM

Gentle Ping.

The intrinsic map is added in https://reviews.llvm.org/D125519 which dependent on this PR.

bogner accepted this revision.Jun 14 2022, 12:22 PM
bogner added a subscriber: bogner.

LGTM with a few minor nitpicks / clean ups

llvm/lib/Target/DirectX/DXIL.td
32

Why is this comment quoted?

llvm/test/CodeGen/DirectX/umax.ll
5 ↗(On Diff #434725)

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

11 ↗(On Diff #434725)

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.

34 ↗(On Diff #434725)

You can delete the llvm.module.flags and llvm.ident metadata, it won't affect this test

This revision is now accepted and ready to land.Jun 14 2022, 12:22 PM

Clean comments and test.

python3kgae marked 4 inline comments as done.Jun 14 2022, 2:03 PM

Updated the comments and test.
Thanks for the review.

This revision was landed with ongoing or failed builds.Jun 14 2022, 5:32 PM
This revision was automatically updated to reflect the committed changes.