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

Inline all functions that are called. #898

Merged
merged 1 commit into from
Oct 24, 2024
Merged

Conversation

LPanosTT
Copy link
Contributor

@LPanosTT LPanosTT commented Oct 10, 2024

  • Register FuncDialect Inliner interface
  • Create and register TTIRDialect Inliner interface
  • Added pass that will make-private all functions which are called
    • The inliners will inline all functions - however it will leave those functions in the module if they are public even when they are dead code. Setting them to private ensures they are deleted from the module.

Since we are inlining all functions that are called, if the module has multiple distinct programs they will all remain afterward. This is because the topmost function in the call stack is the only non-dead-code function that doesn't get called.

Disabling the inliner pass is as simple as removing the added lines in lib/Dialect/TTNN/Pipelines/TTNNPipelines.cpp

If you wish to not register them altogether you'll have to delete their invocations in lib/RegisterAll.cpp::registerAllExtensions

@LPanosTT LPanosTT force-pushed the lpanos/inline_functions branch from 277be35 to de1e98a Compare October 10, 2024 19:09
@LPanosTT LPanosTT changed the title Inline all functions besides main function Inline all functions that are called. Oct 10, 2024
@LPanosTT LPanosTT force-pushed the lpanos/inline_functions branch from de1e98a to 2696df8 Compare October 10, 2024 19:14
@mtopalovicTT
Copy link
Contributor

Out of curiosity, will forge ever generate something like below?

%1 = call @do_mult(%arg0, %arg1, %0) : (tensor<64x128xf32>, tensor<64x128xf32>, tensor<64x128xf32>) -> tensor<64x128xf32>

lib/Dialect/TTIR/Transforms/Passes.cpp Outdated Show resolved Hide resolved
lib/RegisterAll.cpp Outdated Show resolved Hide resolved
Comment on lines 53 to 56
registry.addExtension<TTIRDialect>(
+[](MLIRContext *ctx, TTIRDialect *dialect) {
dialect->addInterfaces<TTIRInlinerInterface>();
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this have to be defined as an extension? Or could you just do

addInterfaces<TTIRInlinerInterface>();

at the dialect initialization.

Copy link
Contributor Author

@LPanosTT LPanosTT Oct 11, 2024

Choose a reason for hiding this comment

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

I could do that. But for FuncDialect I’m forced to register it since its initialize method is inside the toolchain. So, I thought it would be cleaner to just explicitly register them both rather than register only the FuncDialect Inliner and add the TTIRDialect Inliner in Initialize()

Copy link
Contributor

Choose a reason for hiding this comment

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

I see your point. I am not sure should we do it this way, as extensions are used to extend some dialect that is already built for you. Maybe I am missing knowledge on dialect extensions. @sdjordjevicTT what do you think?

Btw func inliner depends on CF dialect, and creates CF branch op.

  void handleTerminator(Operation *op, Block *newDest) const final {
...
    builder.create<cf::BranchOp>(op->getLoc(), newDest, returnOp.getOperands());

are we sure this op won't be inserted in our IR? We have no lowering for CF dialect for any backend.

Copy link
Contributor Author

@LPanosTT LPanosTT Oct 11, 2024

Choose a reason for hiding this comment

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

I believe that only hits for functions defined inside other functions or as lambdas to certain ops. Which I guess could be a problem. Another idea is to create our own Call, Func, and Return ops in TTIR so we don't need to rely on the Func dialect at all, and we can just use the Inliner I've provided. When lowering to TTIR from whatever frontend we'd just need to replace the Func::FuncOp generation with TTIR::FuncOp etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I can also define my own Inliner for the func dialect (which wouldn't have the overloaded handleTerminator that creates cf ops) and register that instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

It does seem like arith dialect uses the interface approach, can we experiment with that @LPanosTT?

 namespace {                                         
  /// This class defines the interface for handling inlining for arithmetic         
  /// dialect operations.                         
  struct ArithInlinerInterface : public DialectInlinerInterface {    
    using DialectInlinerInterface::DialectInlinerInterface;    
                                                  
    /// All arithmetic dialect ops can be inlined.                              
    bool isLegalToInline(Operation *, Region *, bool, IRMapping &) const final {    
      return true;                                                              
    }                                                                           
  };                                                                            
  } // namespace 

  void arith::ArithDialect::initialize() {                                      
    ...                                                                                                                                                                                                                               
    addInterfaces<ArithInlinerInterface>();

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah torch mlir does it this way too: https://github.com/llvm/torch-mlir/blob/8787970afed3c4e1497fb24c4fdeec179fcb61f6/lib/Dialect/Torch/IR/TorchDialect.cpp#L32

I think it seems like it can be done with only changes to TTIRDialect.cpp. I'm not sure we need to register the inliner on behalf of func.

Copy link
Contributor

Choose a reason for hiding this comment

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

See their InitAll for reference too https://github.com/llvm/torch-mlir/blob/8787970afed3c4e1497fb24c4fdeec179fcb61f6/lib/InitAll.cpp#L13.

It seems like all they do is:

  • Implement the dialect interface
  • addInterfaces
  • Register func dialect inliner extension

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nsmithtt it looks like they do register the inliner on behalf of func though

void mlir::torch::registerAllExtensions(mlir::DialectRegistry &registry) {
  mlir::func::registerInlinerExtension(registry);
  tensor::registerInferTypeOpInterfaceExternalModels(registry);
}

I can move the registration of the TTIR Inliner interface to TTIRDialect.cpp and use addInterfaces I've tried and it works. So I'll do that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nsmithtt @rpavlovicTT Should I create my own inliner for func? One that doesn't override the handleTerminator for Block? Just so that we leave the cf dialect entirely out of the picture.

@LPanosTT
Copy link
Contributor Author

LPanosTT commented Oct 11, 2024

Out of curiosity, will forge ever generate something like below?

@mtopalovicTT I don’t think forge will since we only have one forge graph and no 'func' ops. However JAX certainly can (I.e relu can be put in a separate function defined as a maximum against 0). I think it depends on the actual structure of the model in JAX and if there are functions that are called in the python definition

@LPanosTT LPanosTT force-pushed the lpanos/inline_functions branch from 7451218 to 4da241c Compare October 11, 2024 13:50
Comment on lines 53 to 56
registry.addExtension<TTIRDialect>(
+[](MLIRContext *ctx, TTIRDialect *dialect) {
dialect->addInterfaces<TTIRInlinerInterface>();
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah torch mlir does it this way too: https://github.com/llvm/torch-mlir/blob/8787970afed3c4e1497fb24c4fdeec179fcb61f6/lib/Dialect/Torch/IR/TorchDialect.cpp#L32

I think it seems like it can be done with only changes to TTIRDialect.cpp. I'm not sure we need to register the inliner on behalf of func.

@LPanosTT LPanosTT force-pushed the lpanos/inline_functions branch 2 times, most recently from 132b8c5 to bce63ef Compare October 11, 2024 15:32
Copy link
Contributor

@nsmithtt nsmithtt left a comment

Choose a reason for hiding this comment

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

Thanks @LPanosTT! This looks great

@LPanosTT LPanosTT force-pushed the lpanos/inline_functions branch from bce63ef to e0b227f Compare October 16, 2024 14:25
@nsmithtt
Copy link
Contributor

@LPanosTT, I think I have a build fix over here:

I've only tested on macOS so far, still need to get ubuntu to sign off and ideally one of the FE CI's to run against it.

@LPanosTT LPanosTT force-pushed the lpanos/inline_functions branch from e0b227f to 816aa8a Compare October 22, 2024 14:49
@LPanosTT LPanosTT mentioned this pull request Oct 22, 2024
@LPanosTT LPanosTT force-pushed the lpanos/inline_functions branch from 816aa8a to 66df189 Compare October 23, 2024 15:09
@LPanosTT LPanosTT force-pushed the lpanos/inline_functions branch from 66df189 to 03dd47e Compare October 23, 2024 15:24
@LPanosTT LPanosTT enabled auto-merge (squash) October 23, 2024 15:32
@nsmithtt
Copy link
Contributor

fyi @wooseokTT

@LPanosTT LPanosTT disabled auto-merge October 23, 2024 18:43
@LPanosTT LPanosTT merged commit 2571a98 into main Oct 24, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants