Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement pretty printing for SelectOp, ClampOp, AfterAllOp types. Clean up type prints. #37

Merged
merged 5 commits into from
Sep 2, 2022

Conversation

GleasonK
Copy link
Member

@GleasonK GleasonK commented Aug 24, 2022

Pretty print for SelectOp based on vector display of arith.select: https://mlir.llvm.org/docs/Dialects/ArithmeticOps/#arithselect-mlirarithselectop

// Element-wise vector selection in arith:
%vx = arith.select %vcond, %vtrue, %vfalse : vector<42xi1>, vector<42xf32>

// StableHLO display:
%0 = stablehlo.select %arg0, %arg1, %arg1 : tensor<2x3xi1>, tensor<2x3xi32>

Note that if branch types do not match exactly, functional-type is used:

%1 = stablehlo.select %arg0, %arg2, %arg3 : (tensor<2x3xi1>, tensor<2x?xi32>, tensor<?x2xi32>) -> tensor<2x?xi32>

Now that SameOperandsAndResultType is merged into main, we can use it in other ops that may have the same operands and result types like ClampOp, or ops that must have matching types like AfterAllOp:

%0 = stablehlo.clamp %arg0, %arg0, %arg0 : tensor<4xf32>
%5 = stablehlo.after_all %arg1, %arg1 : !stablehlo.token
%6 = stablehlo.after_all : !stablehlo.token

Added print tests and invalid MLIR tests.

Additionally this change cleans up some type prints, removing parentheses around operands / including parens around inputs in type printing for consistency with the rest of the dialect's assembly:

%0 = stablehlo.cstr_reshapable %arg0, %arg1 : (index, tensor<3xi32>) -> !shape.witness
%0 = stablehlo.compute_reshape_shape %arg0, %arg1 : (index, tensor<2xi32>) -> tensor<2xi32>
%0 = stablehlo.complex %arg0, %arg0 : (tensor<4xf32>, tensor<4xf32>) -> tensor<4xcomplex<f32>>

@GleasonK GleasonK requested a review from burmako August 24, 2022 18:19
@GleasonK GleasonK changed the title Implement pretty printing for SelectOp. Use condensed type for ClampOp and AfterAllOp. Implement pretty printing for SelectOp, ClampOp, AfterAllOp types. Clean up type prints. Aug 24, 2022
@GleasonK GleasonK force-pushed the pretty-types-v2 branch 3 times, most recently from 218cfd7 to 4802e97 Compare August 26, 2022 15:45
stablehlo/dialect/StablehloOps.td Outdated Show resolved Hide resolved
stablehlo/dialect/StablehloOps.td Show resolved Hide resolved
stablehlo/dialect/StablehloOps.cpp Outdated Show resolved Hide resolved
stablehlo/dialect/StablehloOps.cpp Outdated Show resolved Hide resolved
stablehlo/dialect/StablehloOps.cpp Outdated Show resolved Hide resolved
stablehlo/dialect/StablehloOps.cpp Show resolved Hide resolved
stablehlo/dialect/StablehloOps.cpp Outdated Show resolved Hide resolved
stablehlo/dialect/StablehloOps.cpp Outdated Show resolved Hide resolved
stablehlo/tests/print_stablehlo.mlir Show resolved Hide resolved
SmallVectorImpl<OpAsmParser::UnresolvedOperand>& operands,
SmallVectorImpl<Type>& opTypes, Type& result) {
// Insert a type for each operand. Need to do this since passing the type of
// a variadic op gives no indication of how many operands were provided.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oof this is unfortunate. Let me take a closer look at this during the next pass over this PR. I'll submit my review for now because there's already plenty of comments to address.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been staring at how this function creates a temporary vector of pointers and realized that reuse is probably overrated in this case. Then I rewrote this function without leveraging parseSameOperandsAndResultTypeImpl, and I think I like the result better:

ParseResult parseVariadicSameOperandsAndResultType(
    OpAsmParser& parser,
    SmallVectorImpl<OpAsmParser::UnresolvedOperand>& operands,
    SmallVectorImpl<Type>& opTypes, Type& result) {
  llvm::SMLoc loc = parser.getCurrentLocation();

  Type type;
  if (parser.parseType(type)) {
    return failure();
  }

  // Handle if function type, all operand types did not match result type.
  if (auto fnType = type.dyn_cast<FunctionType>()) {
    if (fnType.getResults().size() != 1) {
      return parser.emitError(loc, "expected single output");
    }
    llvm::append_range(opTypes, fnType.getInputs());
    result = fnType.getResults()[0];
    return success();
  }

  // Handle bare types. ` : type` indicating all input/output types match.
  opTypes.append(operands.size(), type);
  result = type;
  return success();
}

Yes, there is some duplication with parseSameOperandsAndResultTypeImpl (more precisely, the parseType and dyn_cast and fnType.getResults().size() != 1 parts), but I think this version of code is easier to understand. What's yout take on this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm.. So the real advantage of reuse in printer/parser code in my opinion is the parity:

let x = printSameOperandsAndResultTypeImpl(input)
let y = parseSameOperandsAndResultTypeImpl(x)
assert(y == input)

I feel like there is extra gravity to code duplication in printer/parser code. Duplicating this code adds the overhead of "if we change anything about the printer code, we need to update the parser code in two places." I'll keep thinking on how to improve this.

Copy link
Contributor

@burmako burmako left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like I clicked "Comment" in my previous review, instead of "Request changes".

stablehlo/dialect/StablehloOps.cpp Outdated Show resolved Hide resolved
stablehlo/dialect/StablehloOps.cpp Show resolved Hide resolved
stablehlo/dialect/StablehloOps.cpp Outdated Show resolved Hide resolved
stablehlo/dialect/StablehloOps.cpp Outdated Show resolved Hide resolved
stablehlo/dialect/StablehloOps.cpp Outdated Show resolved Hide resolved
stablehlo/dialect/StablehloOps.cpp Outdated Show resolved Hide resolved
stablehlo/dialect/StablehloOps.cpp Show resolved Hide resolved
stablehlo/dialect/StablehloOps.cpp Outdated Show resolved Hide resolved
Use SameOperandsAndResultType for operations that may have single type
(ClampOp, AfterAllOp).
@GleasonK GleasonK merged commit e408d65 into openxla:main Sep 2, 2022
@GleasonK GleasonK deleted the pretty-types-v2 branch September 6, 2022 19:30
@burmako burmako added the Migrate to MHLO PR that needs to be migrated to MLIR-HLO label Oct 6, 2022
copybara-service bot pushed a commit to jax-ml/jax that referenced this pull request Oct 26, 2022
…shapeOp, ComputeReshapeShapeOp, SelectOp.

Based on:
openxla/stablehlo#37

PiperOrigin-RevId: 466802406
copybara-service bot pushed a commit to jax-ml/jax that referenced this pull request Oct 26, 2022
…shapeOp, ComputeReshapeShapeOp, SelectOp.

Based on:
openxla/stablehlo#37

PiperOrigin-RevId: 466802406
copybara-service bot pushed a commit to jax-ml/jax that referenced this pull request Oct 26, 2022
…shapeOp, ComputeReshapeShapeOp, SelectOp.

Based on:
openxla/stablehlo#37

PiperOrigin-RevId: 466802406
copybara-service bot pushed a commit to jax-ml/jax that referenced this pull request Oct 27, 2022
…shapeOp, ComputeReshapeShapeOp, SelectOp.

Based on:
openxla/stablehlo#37

PiperOrigin-RevId: 466802406
copybara-service bot pushed a commit to jax-ml/jax that referenced this pull request Oct 27, 2022
…shapeOp, ComputeReshapeShapeOp, SelectOp.

Based on:
openxla/stablehlo#37

PiperOrigin-RevId: 466802406
copybara-service bot pushed a commit to jax-ml/jax that referenced this pull request Oct 27, 2022
…shapeOp, ComputeReshapeShapeOp, SelectOp.

Based on:
openxla/stablehlo#37

PiperOrigin-RevId: 466802406
copybara-service bot pushed a commit to jax-ml/jax that referenced this pull request Oct 27, 2022
…shapeOp, ComputeReshapeShapeOp, SelectOp.

Based on:
openxla/stablehlo#37

PiperOrigin-RevId: 466802406
copybara-service bot pushed a commit to tensorflow/mlir-hlo that referenced this pull request Oct 27, 2022
…shapeOp, ComputeReshapeShapeOp, SelectOp.

Based on:
openxla/stablehlo#37

PiperOrigin-RevId: 484271777
copybara-service bot pushed a commit to jax-ml/jax that referenced this pull request Oct 27, 2022
…shapeOp, ComputeReshapeShapeOp, SelectOp.

Based on:
openxla/stablehlo#37

PiperOrigin-RevId: 484271777
copybara-service bot pushed a commit to tensorflow/tensorflow that referenced this pull request Oct 27, 2022
…shapeOp, ComputeReshapeShapeOp, SelectOp.

Based on:
openxla/stablehlo#37

PiperOrigin-RevId: 484271777
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Migrate to MHLO PR that needs to be migrated to MLIR-HLO Prettyprinting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants