Page MenuHomePhabricator

fix DRR's optional operand support
Needs ReviewPublic

Authored by yaochengji on Jun 6 2021, 6:50 PM.


include "mlir/IR/"

def Test_Dialect : Dialect {
  let name = "test";
class NS_Op<string mnemonic, list<OpTrait> traits> :
    Op<Test_Dialect, mnemonic, traits>;

def EOp : NS_Op<"e_op", []> {
  let arguments = (ins
    AnyInteger: $any_integer,
    Optional<AnyInteger>: $optioanl_integer

def FOp : NS_Op<"f_op", []> {
  let arguments = (ins
    AnyInteger: $any_integer,
    Optional<AnyInteger>: $optioanl_integer

// CHECK: if (auto tmpOperand = (*arg0.begin())) {
// CHECK: if (auto tmpOperand = (arg1.size() > 0? (*arg1.begin()): nullptr)) {
def test5 : Pat<(EOp $arg0, $arg1), (FOp $arg0, $arg1)>;

Without the patch, tblgen would generate cpp code like

arg1 = castedOp0.getODSOperands(1);

and cause compilation error. Here arg1 is type ValueRange and tblgen_values is type SmallVector.

Diff Detail

Event Timeline

yaochengji created this revision.Jun 6 2021, 6:50 PM
yaochengji requested review of this revision.Jun 6 2021, 6:50 PM
yaochengji edited the summary of this revision. (Show Details)Jun 6 2021, 6:52 PM
yaochengji edited the summary of this revision. (Show Details)Jun 6 2021, 6:55 PM
bondhugula requested changes to this revision.Jun 11 2021, 4:54 PM
bondhugula added a subscriber: bondhugula.
  1. Please add a test case.
  1. Could you update the commit summary to also describe what it's fixing?
This revision now requires changes to proceed.Jun 11 2021, 4:54 PM
jpienaar added inline comments.

I don't know if this will work: if you have two optional args next to each other, the caller would not be able to differentiate which is specified. It would seem that passing in nullptr for those and handling it in the builder would work instead and keep ordering.

yaochengji added inline comments.Jun 12 2021, 8:09 AM

Yeah, it won't work for two optional args. Let me figure out a more reasonable fix.

yaochengji edited the summary of this revision. (Show Details)

add an unittest and update summary.

yaochengji added inline comments.Jun 14 2021, 10:49 AM

After studying the code, I find that a bottom-up change is required to make things right. You could take a look at OperationState::addOperands, it could not differentiate multi optional operands, either. Neither could the automatically generated Op' build method.

I think this patch could resolve some compilation problems of the previous version of generated cpp code. Making the tblgen handle all sort of optional operands situation is beyond its scope.

change nullptr to Value()

bondhugula resigned from this revision.Jun 15 2021, 10:22 AM

I just had superficial comments. Please do go with @jpienaar or one of the other reviewers' comments.