search

tensorflow / mlir

"Multi-Level Intermediate Representation" Compiler Infrastructure
1.74k stars 260 forks source link

[spirv] Add binary logical operations. #61

Closed denis0x0D closed 5 years ago

denis0x0D commented 5 years ago

This patch adds binary logical operations regarding to the spec section 3.32.15: OpIEqual, OpINotEqual, OpUGreaterThan, OpSGreaterThan, OpUGreaterThanEqual, OpSGreaterThanEqual, OpULessThan, OpSLessThan, OpULessThanEqual, OpSLessThanEqual, this ops are useful for integer comparison.

I've splitted bianary ops to logical ops and arithmetic ops. Also added custom parser, since default one does not fit the requirements (result type is different to operands type), and printer, verifier.

Tests added to ops.mlir and to serialization test machinery. Some tests related to arithmetic ops in ops.mlir reordered to the lexicographic order, since I was mistaken in previous commit.

@antiagainst @MaheshRavishankar can you please take a look? Thanks.

antiagainst commented 5 years ago

Hey @denis0x0D, one question regarding the define_inst.sh script. I think it does not handle the case of subclassing and omitting arguments/results well. Have you experienced that? Did you fixed it or something? Would be nice to upstream if you already got a fix for it. :)

MaheshRavishankar commented 5 years ago

No more comments from me. Thanks!

denis0x0D commented 5 years ago

@antiagainst @MaheshRavishankar thanks a lot for review! @antiagainst it does not work with subclasses, I've already generated a susbset of operation, before the commit with arithmetic operations. I did not have a fix for this moment, but I can take a look, since it saves a lot of time.

antiagainst commented 5 years ago

Oh, I see. It also might be easier if we create a new .td file for these binary ops and move them there so the main SPIRVOps.td file can use the existing script.

antiagainst commented 5 years ago

But it does mean we may not have the automatic updates of documentation on those binary ops. But that would be fine for now given that the spec is fairly stable and changes with low frequency.

denis0x0D commented 5 years ago

@antiagainst if it's ok, I can move them to a new *.td, with the next patch for other bin operations.

antiagainst commented 5 years ago

SGTM! Please have a PR for just moving stuff so it's easier to review. :)

denis0x0D commented 5 years ago

@antiagainst got it, thanks!