Page MenuHomePhabricator

[mlir] expose affine map to C API
ClosedPublic

Authored by zhanghb97 on Mon, Sep 14, 9:02 AM.

Details

Summary

This patch provides C API for MLIR affine map.

  • Implement C API for AffineMap class.
  • Add Utils.h to include/mlir/CAPI/, and move the definition of the CallbackOstream to Utils.h to make sure mlirAffineMapPrint work correct.
  • Add TODO for exposing the C API related to AffineExpr and mutable affine map.

Diff Detail

Event Timeline

zhanghb97 created this revision.Mon, Sep 14, 9:02 AM
Herald added a project: Restricted Project. · View Herald TranscriptMon, Sep 14, 9:02 AM
zhanghb97 requested review of this revision.Mon, Sep 14, 9:02 AM
ftynse requested changes to this revision.Mon, Sep 14, 9:45 AM
ftynse added inline comments.
mlir/include/mlir-c/AffineMap.h
21

I'd call this mlirAffineMapEmptyGet.

25

This should be the default mlirAffineMapGet IMO.

26

We agreed to use intptr_t instead of unsigned for anything related to sizes. Dim/symbol count are sizes.

30

I would not not repeat Map in the name, mlirAffineMapConstantGet should be enough.

43

Please give more details on what should the arguments be.

mlir/include/mlir-c/IR.h
364–386 ↗(On Diff #291586)

Why can all of this go to AffineMap.h ?

mlir/include/mlir/CAPI/IR.h
18 ↗(On Diff #291586)

This inverts the layering IMO. AffineMap should depend on IR, not the inverse.

This revision now requires changes to proceed.Mon, Sep 14, 9:45 AM
zhanghb97 marked 6 inline comments as done.
  • Modify name of mlirAffineMap***Get functions.
  • Use intptr_t for parameters related to sizes.
  • Add details for the comments of the mlirAffineMapPermutationMapGet.
mlir/include/mlir-c/IR.h
364–386 ↗(On Diff #291586)

I noticed that the standard types/attributes place those API in the IR.h, so I followed the rule and I think that placing them in IR.h can maintain consistency with the standard type/attributes.
It's OK for me to place all of them to AffineMap.h, but what about the definition of these functions? The definition of the mlirAffineMapPrint must be placed in the IR.cpp because it needs the CallbackOstream that only available in the IR.cpp. Should we place the mlirAffineMapPrint in IR.cpp alone and place other functions in AffineMap.cpp?

mlir/include/mlir/CAPI/IR.h
18 ↗(On Diff #291586)

Remove the #include, and add the #include "mlir/CAPI/AffineMap.h" to IR.cpp to make sure the functions can work correctly.

ftynse requested changes to this revision.Tue, Sep 15, 1:40 AM
ftynse added inline comments.
mlir/include/mlir-c/AffineMap.h
71–81

These are still unsigned

93

This is still unsigned

mlir/include/mlir-c/IR.h
364–386 ↗(On Diff #291586)

You are mistaking mlir::Attribute, which represents the core concept of an attribute, for _standard attrbutes_, which are specific subclasses of mlir::Attribute such as IntegerAttr or StringAttr. The former, being a core IR concept, is rightfully declared in IR.h. The latter are declared in StandardAttributes.h. IR.h should only contain core IR, i.e. things that are necessary to inspect any operation. Affine maps are not that.

No, the definition of mlirAffineMapPrint can be placed anywhere as long as CallbackOstream is placed in the detail namespace and factored out into a private header that implementations can include.

This revision now requires changes to proceed.Tue, Sep 15, 1:40 AM
zhanghb97 updated this revision to Diff 292078.Tue, Sep 15, 8:07 PM
zhanghb97 marked an inline comment as done.
zhanghb97 edited the summary of this revision. (Show Details)
  • unsigned -> intptr_t
  • Place all the affine map C API back to AffineMap.h/.cpp
  • Add Utils.h and move CallbackOstream to Utils.h to make sure it is available to mlirAffineMapPrint.
mlir/include/mlir-c/AffineMap.h
71–81

Done - unsigned -> intptr_t

93

I think this should be a unsigned type. The resultPos is not a directly used size/index, it is used as the argument of the llvm::makeArrayRef, and the AffineMap::getSubMap(ArrayRef<unsigned> resultPos) method wants a ArrayRef<unsigned>. So if we use intptr_t here, we should convert the intptr_t * to unsigned * when we call the llvm::makeArrayRef to make the getSubMap work. I don't think the conversion is necessary.

mlir/include/mlir-c/IR.h
364–386 ↗(On Diff #291586)

Done - Place the CallbackOstream to mlir/include/mlir/CAPI/Utils.h to make sure a correct layer.

zhanghb97 added inline comments.Tue, Sep 15, 8:13 PM
mlir/include/mlir-c/AffineMap.h
115

See the comment above for the reason I use unsigned here.

ftynse added inline comments.Wed, Sep 16, 12:55 AM
mlir/include/mlir-c/AffineMap.h
93

Positions in a container should use the same type as the size of said container, don't you think? Yes, you'd need to materialize a new vector and convert elements one by one, we already do this in other places.

zhanghb97 added inline comments.Wed, Sep 16, 2:42 AM
mlir/include/mlir-c/AffineMap.h
93

The resultPos here is the container (not the position in the container), and the unsigned type here is the type of elements in the container (see the method AffineMap::getSubMap(ArrayRef<unsigned> resultPos)). The type of element has no relationship with the type of size, right?

For example:
The int64_t *shape in the mlirVectorTypeGet

MlirType mlirVectorTypeGet(intptr_t rank, int64_t *shape, MlirType elementType);

is similar with the unsigned *resultPos here:

MlirAffineMap mlirAffineMapGetSubMap(MlirAffineMap affineMap, intptr_t size,
                                     unsigned *resultPos);

The type of the shape or resultPos depends on the method:

VectorType::get(ArrayRef<int64_t> shape, Type elementType)

or

AffineMap::getSubMap(ArrayRef<unsigned> resultPos)

Is my understanding correct? Or I missed somthing important?

Apart from that, if we use intptr_t here, I think the conversion can be:

MlirAffineMap mlirAffineMapGetSubMap(MlirAffineMap affineMap, intptr_t size,
                                     intptr_t *resultPos) {
  return wrap(unwrap(affineMap).getSubMap(
      llvm::makeArrayRef((unsigned *)resultPos, static_cast<size_t>(size))));
}

why should we materialize a new vector?

Is my understanding correct? Or I missed somthing important?

You are missing the fact that positions of you pass into the call are merely indices of a vector. AffineMap is slightly more than just a vector of AffineExpr. AffineMap::getNumResults returns the size of that vector. If that size has intptr_t type, so should the indices. Type difference also makes user code full of casts that can be avoided.

Apart from that, if we use intptr_t here, I think the conversion can be:

 MlirAffineMap mlirAffineMapGetSubMap(MlirAffineMap affineMap, intptr_t size,
                                     intptr_t *resultPos) {
  return wrap(unwrap(affineMap).getSubMap(
      llvm::makeArrayRef((unsigned *)resultPos, static_cast<size_t>(size))));
}

why should we materialize a new vector?

Because you cannot just cast intptr_t * to unsigned * and expect to access the same data. The pointed-to types may or may not have the same bitwidth. And such casts may be forbidden under strict aliasing.

zhanghb97 updated this revision to Diff 292197.Wed, Sep 16, 5:58 AM
  • Use intptr_t * for the `resultPos parameter in mlirAffineMapGetSubMap function.
  • Use a vector to cast the element of resultPos from intptr_t to unsigned.
ftynse accepted this revision.Wed, Sep 16, 7:08 AM

Thanks for iterating!

This revision is now accepted and ready to land.Wed, Sep 16, 7:08 AM
This revision was automatically updated to reflect the committed changes.

Hi @ftynse - After pushing this patch, I received en email said that:

The Buildbot has detected a new failure on builder mlir-windows while building llvm.
Full details are available at:
http://lab.llvm.org:8011/builders/mlir-windows/builds/7787

I don't know why I received this, what can I do for it?

I don't think that is related to you: that run was for a batch of changes
and the error is in a section of the tree in no way related to your change.
I think you're clear.

bondhugula added inline comments.Thu, Sep 17, 2:15 AM
mlir/test/CAPI/ir.c
597

Document the return value?

825–834

Should all the trailing ones be CHECK-NEXT?

ftynse added inline comments.Thu, Sep 17, 3:58 AM
mlir/test/CAPI/ir.c
825–834

No. This would add an unnecessary constraint (no lines printed in between) that is irrelevant to what is being tested. Labels suffice to differentiate between different "blocks" of tests.

835

Don't forget to turn clang-format back on again.

zhanghb97 added inline comments.Thu, Sep 17, 5:59 PM
mlir/test/CAPI/ir.c
835

OK, I will fix this with the next patch about MutableAffineMap.

ftynse added inline comments.Fri, Sep 18, 1:47 AM
mlir/test/CAPI/ir.c
835

I have already changed this.

zhanghb97 added inline comments.Fri, Sep 18, 2:00 AM
mlir/test/CAPI/ir.c
835

Thanks!