This is an archive of the discontinued LLVM Phabricator instance.

[mlir] Expose affine expression to C API
ClosedPublic

Authored by zhanghb97 on Oct 21 2020, 12:52 AM.

Details

Summary

This patch provides C API for MLIR affine expression.

  • Implement C API for methods of AffineExpr class.
  • Implement C API for methods of derived classes (AffineBinaryOpExpr, AffineDimExpr, AffineSymbolExpr, and AffineConstantExpr).
  • Expose IsA and Get methods for each affine binary operation expression.

Diff Detail

Event Timeline

zhanghb97 created this revision.Oct 21 2020, 12:52 AM
zhanghb97 requested review of this revision.Oct 21 2020, 12:52 AM
ftynse added inline comments.Oct 21 2020, 3:42 AM
mlir/include/mlir-c/AffineExpr.h
23

I am strongly interested in changing this and not exposing the enum here, even though the C++ API does. To me, this feels like an early design iteration that was never migrated to the typeid/isa scheme. Could we remove this, the GetKind method and instead provide: mlirAffineExprIsADim, mlirAffineExprIsAFloorDiv for checking (including the top-level mlirAffineExprIsABinaryExpr that matches all binary expressions) and mlirAffineFoorDivExprGet etc. that match the already existing mlirAffineDimExprGet?

WDYT? @stellaraccident?

mlir/lib/CAPI/IR/AffineExpr.cpp
123

I'd suggest adding a helper function that takes MlirAffineExprKind and returns an mlir::AffineExprKind. Then this function reduces to return wrap(getAffineBinaryOpExpr(convertKind(kind), unwrap(lhs), unwrap(rhs));

zhanghb97 added inline comments.Oct 21 2020, 8:46 AM
mlir/include/mlir-c/AffineExpr.h
23

I noticed that Diagnostics C API exposes the enum MlirDiagnosticSeverity so I used the enum here because these two are similar. Now I think the isa scheme is more consistent with other APIs. Looking forward to Stella's opinion.
I also want to confirm that if we use the scheme, we should

  • remove the enum
  • change the getKind to IsA for each kind
  • change mlirAffineBinaryOpExprGet to mlirAffine***ExprGet for each binary operation.

Is that correct?

ftynse added inline comments.Oct 21 2020, 11:58 AM
mlir/include/mlir-c/AffineExpr.h
23

Yes, I guessed that you followed the diagnostics. The reason why I don't like it for the AffineExpr hierarchy is that it does have subclasses, but only for some expressions (e.g., AffineDimExpr is a proper subclass, but AffineAddExpr is not). And worse, the kind enum serves both for subclassing purposes and for further subdivision within a class. This creates all sorts of problems about what should be handled when, leading to non-exhaustive switch conditions like you probably noticed given the number of llvm_unreachable you had to include.

And yes, your understanding of my proposal is correct.

zhanghb97 updated this revision to Diff 300194.Oct 23 2020, 1:38 AM
zhanghb97 edited the summary of this revision. (Show Details)

Rebase.

Use the typeid/isa scheme, expose IsA and Get methods for each affine binary operation expression.

ftynse accepted this revision.Oct 23 2020, 4:39 AM
This revision is now accepted and ready to land.Oct 23 2020, 4:39 AM
This revision was landed with ongoing or failed builds.Oct 23 2020, 5:07 AM
This revision was automatically updated to reflect the committed changes.