[MLIR] Move eraseArguments and eraseResults to FunctionLike

[mikeurbach]

Summary

Previously, they were only defined for FuncOp.

To support this, FunctionLike needs a way to get an updated type
from the concrete operation. This adds a new hook for that purpose,
called getTypeWithoutArgsAndResults.

For now, FunctionLike continues to assume the type is
FunctionType, and concrete operations that use another type can hide
the getType, setType, and getTypeWithoutArgsAndResults methods.

[mikeurbach]

I took a slightly different approach here compared to the previous implementation. Rather than requiring a new hook on the concrete ops, I just added a member function on FunctionLike and defined it in terms of FunctionType. To me, this isn't much different than how getType and setType work today. By doing it this way, existing FunctionLike ops do not need to change, and can immediately access the new functionality. However, this doesn't really fix the assumption we are using FunctionType, and indeed perpetuates it. I updated the comments to try to be more explicit about this and what concrete ops need to do if they want this functionality and want to use their own function type.

This is a little different that what we originally discussed, so I'm curious what you all think.

@ftynse After doing things this way, I ended up removing all the changes related to LLVMFuncOp. It already doesn't define a setType member function because it doesn't use that functionality. Since it isn't using the functionality extracted from FuncOp, I didn't think it was necessary to define setType or getTypeWithoutArgsAndResults. If it's preferable, I can add back the change that defines these for LLVMFuncOp for compatibility. I also took a look at updating the return type of getType, and that can be done smoothly--call sites are using getFunction* helpers on LLVMType and are straightforward to update... I could submit a separate patch for that.

mlir/include/mlir/IR/FunctionSupportImplementation.h

2

@rriddle I'm not sure if this is what you had in mind, but I put the functions removed from FuncOp here. I wasn't sure about the implementations that were already in FunctionSupport.h, but this lets me share these functions between FuncOp and the ops I'm working with out of tree.

mlir/lib/Dialect/Linalg/Transforms/DropUnitDims.cpp

168

@mravishankar here is what I ended up adding. I was having a trouble figuring out how to pass this DenseSet<unsigned> without copying the indices into a vector here. I expected the following to work, but the DenseSet const iterator types weren't compatible:

ArrayRef<unsigned> unitDimsToErase(unitDims.begin(), unitDims.end())

Is there a better way to pass the indices in this set as an ArrayRef? In general, is ArrayRef the right API to expose here?

[mikeurbach]

Add missing header guard to new header file.

[ftynse]

The approach LGTM.
Deferring to @rriddle on header layering.

mlir/include/mlir/IR/FunctionSupportImplementation.h

2

I am not convinced by having two headers and requiring the user to include an extra header...

mlir/lib/IR/Types.cpp

67–70

This looks like a repetitive pattern. Would it make sense to define a utility function (somewhere in lib/Support) along the lines of

template <typename Range>
BitVector bitVectorSetAtPositions(unsigned max, Range &&range) {
  BitVector result(max);
  for (unsigned i : range)
    result.set(i);
  return result;
}

[mikeurbach]

mlir/lib/IR/Types.cpp

67–70

I think it would make sense. I can a helper like that and use it here.

[mravishankar]

THanks for moving the method to Block. Other knowledgable people can comment on rest of the patch.

[mikeurbach]

Moved BitVector creation into a helper in Support.

[rriddle]

mlir/include/mlir/IR/FunctionSupportImplementation.h

2

This isn't exactly what I had in mind. I would imagine that we could just have a FunctionSupport.cpp file with type erased methods that are called by the templated version. For example, looking at eraseArguments the only thing that relies on a template is getTypeWithoutArgsAndResults, the rest we could write in a type erased fashion(Updating the attributes and the block arguments don't require anything from the template).

mlir/include/mlir/Support/ADTExtras.h

22 ↗

(On Diff #301710)

nit: This code block doesn't look large enough to justify a special method. We also prefer to keep mlir/Support as free as possible of things that could go directly into llvm/support.

mlir/lib/IR/Block.cpp

180

Maybe have a bitvector variant so we don't need to create multiple bitvectors?

mlir/lib/IR/Types.cpp

59

Can you avoid all of this if the argIndices or resultIndices are empty?
e.g.

ArrayRef<Type> inputTypes = getInputs();
SmallVector<Type, 4> newInputTypeBuffer;
if (!argIndices.empty()) {
  // Compute the internals of newInputTypeBuffer.
  inputTypes = newInputTypeBuffer;
}

59–64

Same below.

[rriddle]

Apologies for the delay.

[mikeurbach]

mlir/include/mlir/IR/FunctionSupportImplementation.h

2

I see, this makes sense. I'll update this.

I was thinking about getTypeWithoutArgsAndResults... Maybe we can just inline the call to getType().getWithoutArgsAndResults() inside the type erased methods? It should just work for all the concrete ops using FunctionType.

Any ops using another type still have the same "Note that the concrete class must define a method with the same name" caveat they already have for getType. This adds an additional restriction that the type has to have a getWithoutArgsAndResults method.

As was mentioned on Discourse, it would be nice to eventually enforce this kind of thing through a type interface. Perhaps for now, going this route and documenting it in the comments would be sufficient?

mlir/include/mlir/Support/ADTExtras.h

22 ↗

(On Diff #301710)

I can remove it from here. In the places where this pattern is repeated, I guess I'll just define this small helper there.

mlir/lib/IR/Block.cpp

180

Sounds good.

mlir/lib/IR/Types.cpp

59

Yep, I can do that.

[mikeurbach]

mlir/include/mlir/IR/FunctionSupportImplementation.h

2

Ok I realize how the getTypeWithoutArgsAndResults part has to be templated but the rest of the implementation can be done with the type erased. I will keep the getTypeWithoutArgsAndResults helper as it is now.

[mikeurbach]

Refactor implementations into FunctionSupport.cpp, and address other comments.

[mikeurbach]

Remove unused file from previous revision.

[mikeurbach]

@rriddle when you get a chance, please take another look. I think I've addressed your comments from the previous revision.

mlir/include/mlir/IR/FunctionSupport.h

272

@rriddle I hope this is more along the lines of what you had in mind regarding "type erased methods that are called by the templated version". I didn't spend any time refactoring pieces out of FunctionSupport.h into FunctionSupport.cpp, but I put the bulk of the new implementation there.

mlir/lib/IR/Block.cpp

180

@rriddle I added an eraseArguments below that takes a bitvector. Is this what you had in mind? I know you also have concerns with adding more dependencies to headers, and I added BitVector.h for this.

[rriddle]

LGTM after resolving final comments.

mlir/include/mlir/IR/Block.h

18

You don't need to include this here, you can forward declare BitVector and move the include to Block.cpp.

mlir/lib/IR/FunctionSupport.cpp

12 ↗

(On Diff #302659)

Namespaces in source files should only be used for classes, everything else should be top-level:

https://llvm.org/docs/CodingStandards.html#use-namespace-qualifiers-to-implement-previously-declared-functions
https://llvm.org/docs/CodingStandards.html#anonymous-namespaces

MLIR makes use of using namespace mlir to remove the need to fully qualify everything.

mlir/lib/IR/FunctionSupportDetail.h

24 ↗

(On Diff #302659)

I'm a bit apprehensive about adding new files for little helpers like this, can you just duplicate locally in each .cpp for now?

[mikeurbach]

Awesome, thanks for taking a look. Those final comments all make sense.. I will clean those up.

[mikeurbach]

Updates after review.