Page MenuHomePhabricator

[MLIR] Move eraseArguments helper from FuncOp to FunctionLike
AbandonedPublic

Authored by mikeurbach on Oct 23 2020, 2:50 PM.

Details

Summary

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 getTypeWithoutArguments.

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

Diff Detail

Event Timeline

mikeurbach created this revision.Oct 23 2020, 2:50 PM
mikeurbach requested review of this revision.Oct 23 2020, 2:50 PM

I'm not sure if there is a way to mark this as a draft, but please consider it as such. I'm trying to illustrate the ideas discussed here: https://llvm.discourse.group/t/moving-erasearguments-from-funcop-to-functionlike/2016.

ftynse added inline comments.Oct 26 2020, 3:19 AM
mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td
761

This should really be LLVMFunctionType. The LLVMType is used in many places due to legcacy.

mlir/include/mlir/Dialect/LLVMIR/LLVMTypes.h
122

Let's not pollute LLVMType with more methods. The existing methods are mirroring LLVM's API, but new methods are much better off on specific types.

mlir/include/mlir/IR/Function.h
126–127

Nit: this look like irrelevant formatting changes. Does git-clang-format force you to do this because you changed the file?

Can this logic be generalized for all Regions? In the patch https://reviews.llvm.org/D90118 I need to drop arguments for region of an linalg::IndexedGenericOp. Having this method there would be more ergonomic.

Can this logic be generalized for all Regions? In the patch https://reviews.llvm.org/D90118 I need to drop arguments for region of an linalg::IndexedGenericOp. Having this method there would be more ergonomic.

I am thinking about how to generalize this to something useful for your case. In addition to erasing the requisite block arguments, this helper also handles attributes that are specific to FunctionLike ops. The only part that really generalizes to your situation is looping through a set of indices and calling eraseArgument on the Block.

I guess we could add that part to an eraseArguments(ArrayRef<unsigned> indices) method on the Block class. That could be used by eraseArguments on the FunctionLike trait as well as your case. Is that what you mean?

rriddle added inline comments.Oct 26 2020, 1:24 PM
mlir/include/mlir/Dialect/LLVMIR/LLVMTypes.h
336

Comments here should be ///

mlir/include/mlir/Dialect/SPIRV/SPIRVStructureOps.td
266

Comments should be /// here.

mlir/include/mlir/IR/FunctionSupport.h
523

Can you please move as much of this out of line into the .cpp file as possible? Templates end up generating a lot of code, and also require including otherwise unnecessary headers (like BitVector here). (I know this comment also applies to the existing code, but code size is something we should really start paying more attention to).

mlir/include/mlir/IR/Types.h
108

Can you please revert the unrelated formatting changes? Using git-clang-format should help here.

255

Please add a much more detailed comment here, and also use /// for comments.

mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
1465

nit: Spell out the type instead of using auto here.

Thanks for the feedback. A couple questions to inline comments, otherwise I'm addressing the rest of the comments.

mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td
761

Sounds good. Should I also change getType to use LLVMFunctionType at this point or leave it alone?

mlir/include/mlir/IR/FunctionSupport.h
523

I didn't see a .cpp file at lib/IR/FunctionSupport.cpp. Should I create one and put these definitions there?

ftynse added inline comments.Oct 27 2020, 2:25 AM
mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td
761

It would be a nice cleanup if it doesn't require a lot of changes in its call sites..

mikeurbach added inline comments.Oct 27 2020, 8:45 AM
mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td
761

I will try it out, and if it isn't a big mess, include it in the patch.

I guess we could add that part to an eraseArguments(ArrayRef<unsigned> indices) method on the Block class. That could be used by eraseArguments on the FunctionLike trait as well as your case. Is that what you mean?

Yes. That would be useful!

mikeurbach added inline comments.Oct 27 2020, 11:55 AM
mlir/include/mlir/IR/FunctionSupport.h
523

I started looking into this, and now it is making more sense. I'm not a C++ expert, but it looks like these definitions will need to be available to the various places FunctionLike ops use them. I think a good way to achieve that would be to move them into a header here in include/mlir/IR so they can be included in the various .cpp files that need them. Like how there is DialectImplementation.h, OpImplementation.h, etc., there could be a FunctionSupportImplementation.h or something of the sort.

I think this will work, but is that what you had in mind? Is there a better way to do it?

Well, I tried to share my updates and ended up creating a new revision, that is here now: https://reviews.llvm.org/D90363

mikeurbach abandoned this revision.Tue, Nov 3, 4:04 PM

This revision was superseded by https://reviews.llvm.org/D90363.