Page MenuHomePhabricator

[mlir] Introduce bare ptr calling convention for MemRefs in LLVM dialect
ClosedPublic

Authored by dcaballe on Jan 15 2020, 12:52 PM.

Details

Summary

This patch introduces an alternative calling convention for
MemRef function arguments in LLVM dialect. It converts MemRef
function arguments to LLVM bare pointers to the MemRef element
type instead of creating a MemRef descriptor. Bare pointers are
then promoted to a MemRef descriptors at the beginning of the
function. This calling convention is only enabled with a flag.

This is a stepping stone towards having an alternative and simpler
lowering for MemRefs when dynamic shapes are not needed. It can
also be used to temporarily overcome the issue with passing 'noalias'
attribute for MemRef arguments, discussed in [1, 2], since we can
now convert:

func @check_noalias(%static : memref<2xf32> {llvm.noalias = true}) {

return

}

into:

llvm.func @check_noalias(%arg0: !llvm<"float*"> {llvm.noalias = true}) {

%0 = llvm.mlir.undef ...
%1 = llvm.insertvalue %arg0, %0[0] ...
%2 = llvm.insertvalue %arg0, %1[1] ...
...
llvm.return

}

Related discussion:

[1] https://github.com/tensorflow/mlir/issues/309
[2] https://github.com/tensorflow/mlir/pull/337

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Unit tests: pass. 61906 tests passed, 0 failed and 782 were skipped.

clang-tidy: unknown.

clang-format: pass.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

dcaballe updated this revision to Diff 239442.Jan 21 2020, 3:05 PM
  • Move tests for static MemRefs in 'convert-memref-ops.mlir' to 'convert-static-memref-ops.mlir' and enable such tests for bare ptr calling convention.
  • Move file 'convert-memref-ops.mlir' to 'convert-dynamic-memref-ops.mlir'.

This patch is ready for review.

dcaballe edited the summary of this revision. (Show Details)Jan 21 2020, 3:06 PM

Unit tests: pass. 61906 tests passed, 0 failed and 782 were skipped.

clang-tidy: unknown.

clang-format: pass.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

mehdi_amini added inline comments.Jan 22 2020, 11:52 AM
mlir/include/mlir/Conversion/StandardToLLVM/ConvertStandardToLLVM.h
147

I am missing how this gets used (other than in testing with the cl::opt) right now?

mlir/include/mlir/Conversion/StandardToLLVM/ConvertStandardToLLVMPass.h
76

I think this looks better after the refactoring since I'm only introducing convertArgType, which should be a unitary piece of the type conversion and easy to compose with other customizations.

I'm not sure I understand what you mean here: are you saying that LLVMTypeConverter will never ever have any other virtual method that you would need for customization?

I think that breaking LLVM type conversion into smaller unitary chucks would help and this patch goes towards that direction.

How is this patch going in this direction? Maybe I don't perceive the direction you're referring to with "smaller unitary chunks" here?

we could publicly expose only the populate* functions that provide all the conversion patterns (i.e., populateStdToLLVMConversionPatterns and populateStdToLLVMBarePtrConversionPatterns).

Again, I see some composability issue. What about the next orthogonal option? Are we gonna multiply the number of populateStdToLLVM*ConversionPatterns function by two? There is a combinatorial aspect that I don't understand how you see solved.

Unless the "BarePtr" is the only ever customization we want to apply, I don't see how this is scaling forward right now.

Have you looked into injection instead of subclassing here?

dcaballe marked 2 inline comments as done.Jan 22 2020, 7:49 PM

Thanks for the feedback, Mehdi!

The reason for having this upstream is because it's generic enough and there were more people interested in lowering MemRefs to bare pointers in LLVM. It's also useful to temporarily overcome the aliasing issue for function arguments, currently impacting everybody using the default LLVM lowering.
However, if you think this is too problematic I can keep this local to our nGraph repository. I would still need some minor changes upstream to enable the overloading of some methods so that we don't have to duplicate and maintain a lot of code.

Thanks,
Diego

mlir/include/mlir/Conversion/StandardToLLVM/ConvertStandardToLLVM.h
147

That's a good point. I'm currently using it locally by invoking createLowerToLLVM with the provided populateStdToLLVMBarePtrConversionPatterns
makeStandardToLLVMBarePtrTypeConverter. I can add changes and a flag to JitRunner so that we can use it through cpu runner. What do you think?

mlir/include/mlir/Conversion/StandardToLLVM/ConvertStandardToLLVMPass.h
76

I think this looks better after the refactoring since I'm only introducing convertArgType, which should be a unitary piece of the type conversion and easy to compose with other customizations.

I'm not sure I understand what you mean here: are you saying that LLVMTypeConverter will never ever have any other virtual method that you would need for customization?

Not at all! IIUC, your concern about composability was on the first version of the code where I was overloading convertFunctionSignature, a conversion method that does quite a few simple steps altogether (convert argument types, convert return type, create a new function type, etc.). I agree that overloading that method would make composability/reuse of those simple steps difficult. In the second version, I refactored the code that converted argument types into an independent function. By refactoring code into smaller virtual functions doing only a simple thing (that's what I meant by unitary), we should be able to provide a higher level of customization and composability by just overloading the small function that refers to the part of the conversion that we want to customize.

Maybe if you could provide an example of an "orthogonal customization" we could discuss based on something specific.

I think that breaking LLVM type conversion into smaller unitary chucks would help and this patch goes towards that direction.

How is this patch going in this direction? Maybe I don't perceive the direction you're referring to with "smaller unitary chunks" here?

Hopefully it's clearer now with my answer above.

we could publicly expose only the populate* functions that provide all the conversion patterns (i.e., populateStdToLLVMConversionPatterns and populateStdToLLVMBarePtrConversionPatterns).

Again, I see some composability issue. What about the next orthogonal option? Are we gonna multiply the number of populateStdToLLVM*ConversionPatterns function by two? There is a combinatorial aspect that I don't understand how you see solved.

This is a different composability problem but, yes, I agree with you. However, I don't think we are aiming at having every potential combination of customization available. If that were the case, we would have a combinatorial problem regardless of these interfaces being public or not. If we really need a high level of composability here, we could make all the LLVM conversion patterns public so that each client could build its own customized "populate" function. Pinging @ftynse since he also introduced some of these functions.

Have you looked into injection instead of subclassing here?

I assume this refers to the type conversion again. Not sure how injection could improve the design here. We would need either virtual functions or refactoring common code to utility functions. Do you see something I'm missing? Also, type converter is already injected so I think it would make usability more complicated. What do you think?

mlir/include/mlir/Transforms/DialectConversion.h
324

I tried using replaceAllUsesWith but it didn't work. The mapping from 'from' to 'to' is needed. I don't see any other option. Any suggestions?

mehdi_amini added inline comments.Jan 22 2020, 8:20 PM
mlir/include/mlir/Conversion/StandardToLLVM/ConvertStandardToLLVM.h
132

(Just noticed that the comment needs to be updated)

147

Unless I'm missing something, makeStandardToLLVMBarePtrTypeConverter isn't publicly exposed? (it is a static function in the implementation file)

mlir/include/mlir/Conversion/StandardToLLVM/ConvertStandardToLLVMPass.h
76

By refactoring code into smaller virtual functions doing only a simple thing (that's what I meant by unitary), we should be able to provide a higher level of customization and composability by just overloading the small function that refers to the part of the conversion that we want to customize.

Maybe if you could provide an example of an "orthogonal customization" we could discuss based on something specific.

Sure, let me clarify: right now you're overloading convertArgType by adding BarePtrTypeConverter , which only deals with MemRef function arguments.
If later we want to have another customization point to handle conversion for other kind of argument, let say composite type. We'd have to overload the same method in another subclass CustomCompositeTypeConverter. But how would these two subclasses compose with each other? How do I get both the bare memref ABI and the custom composite types?

Expand this to other customization points: assume we want to control the return type of function. We'll create another virtual function similar to convertArgType, let say convertResultType.
We can overload this in subclasses, but again if one subclass is overloading convertResultType but I also want the BarePtrTypeConverter, how do I proceed?

I assume this refers to the type conversion again. Not sure how injection could improve the design here. We would need either virtual functions or refactoring common code to utility functions. Do you see something I'm missing? Also, type converter is already injected so I think it would make usability more complicated. What do you think?

What I'm referring to is that you instead of virtual functions, you can have a callback or a list of callbacks for each of the possible extension point.
For instance instead of having: LLVM::LLVMType convertArgType(Type type); as a virtual method you could store:

std::vector<std::function<LLVM::LLVMType(Type type)>> convertArgTypeImpls;

And have:

LLVM::LLVMType convertArgType(Type type) {
  for (auto &callback : convertArgTypeImpls) {
    type = callback(type);
    // possibly early exit on the first success instead?
  }
  return type;
}

This allows to inject customization both for different types and/or for different customization points (another vector of callbacks for example).

I'm supportive of this change as long as Mehdi's and River's concerns are addressed.

mlir/include/mlir/Conversion/StandardToLLVM/ConvertStandardToLLVMPass.h
76

It sounds like the scalable approach would be to implement type conversion following the similar pattern-based architecture we are building for op conversion... I considered doing that at some point.

An intermediate proposition could be to do the dispatch manually instead of relying on virtual functions with something like

class LLVMTypeCoverter {
  std::function<LLVMType(Type)> memrefConverter;
  std::function<LLVMType(Type)> funcArgumentConverter;
  // ...
};
mlir/lib/Conversion/StandardToLLVM/ConvertStandardToLLVM.cpp
273

I'd rather return null here. This can happen in correct code and we shouldn't crash. Normally, the type conversion should propagate null-as-error-mark.

dcaballe planned changes to this revision.Jan 23 2020, 1:50 PM

Thank you both for the feedback! It makes sense to me now. Let me rework this following your suggestions.
Just to recap, these are the pending issues:

  • LLVMTypeConverter composability/scalability: To be addressed following @mehdi_amini and @ftynse suggestions.
  • populate* functions scalability: No good solution for now. To be addressed in a separate commit (remove most of the populate* functions and make conversion patterns public could work)?
  • Enable bare pointer calling conversion in JITRunner
  • Do not expand scope of replaceUsesOfBlockArgument: Changes on replaceUsesOfBlockArgument seem necessary. Waiting for any other suggestions here (@rriddle?).

Please, let me know if I'm missing something or you have any other concerns before I proceed.
Thanks!

dcaballe updated this revision to Diff 240901.Jan 28 2020, 9:03 AM
dcaballe marked 2 inline comments as done.

Hopefully this approach looks better now! Let's iterate on it.
Changes:

  • Add customization callbacks to LLVMTypeConverter.

    This implementation is a stepping stone towards a pattern-based type conversion approach that allows customization of different points of the type conversion and different types for each customization point.

    Re current implementation:
    • It only allows customization of different points but not customization of different types for each customziation point for now but it should be easy to extend once the latter is needed.
    • LLVMTypeConverterCustomization holds the callbacks for the customizations points available (only function argument types for now) and it's initialized by default to the pre-existing calling convention.
    • 'makeStandardToLLVMBarePtrTypeConverter' is private on purpose to avoid a combinatorial explosion similar to that of populate* functions. Customization methods are made public to allow external clients to create their ad-hoc customizations.
  • Keep 'populateStdToLLVMBarePtrFuncOpConversionPattern'

    This is to reduce the number of public populate* functions. Follow-up work is needed to avoid a potential combinatorial explosion. Probably making rewrite patterns public would help here.
  • Add test with mlir_cpu_runner

    mlir-cpu-runner entry-point invocation is currently limited to a couple of function signatures. For that reason we use a () -> () entry-point function that allocates memrefs, initializes them, passes them to another function and print their output values. Therefore, no changes are needed at JitRunner level for now since the entry-point function won't have memref arguments.

Unit tests: fail. 61903 tests passed, 5 failed and 781 were skipped.

failed: libc++.std/language_support/cmp/cmp_partialord/partialord.pass.cpp
failed: libc++.std/language_support/cmp/cmp_strongeq/cmp.strongeq.pass.cpp
failed: libc++.std/language_support/cmp/cmp_strongord/strongord.pass.cpp
failed: libc++.std/language_support/cmp/cmp_weakeq/cmp.weakeq.pass.cpp
failed: libc++.std/language_support/cmp/cmp_weakord/weakord.pass.cpp

clang-tidy: pass.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

This looks good to me. I don't fully understand the problem @rriddle mentioned with replaceUsesOfWith, and I wonder if we could have a solution that also avoids the need for the placeholder operation.

mlir/include/mlir/Transforms/DialectConversion.h
324

@rriddle could you please elaborate on how exactly it is broken?

mlir/lib/Conversion/StandardToLLVM/ConvertStandardToLLVM.cpp
555–561

Nit: convert input FuncOp to LLVMFuncOp

646

Could we exit early instead?

663

Should we also erase the placeholder op?

This sounds there's a missing feature in the rewriter (or the IR in general) to replace all uses except the given ones...

rriddle added inline comments.Jan 29 2020, 8:30 AM
mlir/include/mlir/Transforms/DialectConversion.h
324

Sorry, I sent this when I was OOO and it got lost in my email. The current problem is that the transformation isn't "revertible". If a pattern that uses it would fail, there isn't a guarantee that the IR will be reverted to its original state. It currently internally directly replaces the uses of the argument, which isn't valid given that everything in DialectConversion has to be undoable.

dcaballe updated this revision to Diff 241258.Jan 29 2020, 12:43 PM
dcaballe marked 4 inline comments as done.

Thanks. Addressing the comments:

  • Rebase
  • Addressed @ftynse comments
  • Revert changes on 'replaceUsesOfBlockArgument'
mlir/include/mlir/Transforms/DialectConversion.h
324

Thanks! Got it now... reverted. I can use replaceUsesOfBlockArgument + rewriter.replaceOp instead for my use case.

mlir/lib/Conversion/StandardToLLVM/ConvertStandardToLLVM.cpp
663

Should we also erase the placeholder op?

I erased it in my previous version but it was a bug. The placeholder must be alive since it goes into rewriter's mapping.

This sounds there's a missing feature in the rewriter (or the IR in general) to replace all uses except the given ones...

We would need something like 'domInstFilter' in (https://github.com/llvm/llvm-project/blob/master/mlir/include/mlir/Transforms/Utils.h#L60) but for the rewritter. I initially considered it but I thought it would be an overkill for this case since we would need to compute dominance information and use dominates queries for each use, which is expensive. Also, dominance information would need to be computed at the time of materializing the replacement (?) instead of when adding the replacement to the map, which would make it more complicated. Maybe something to think about separately?

Unit tests: pass. 62310 tests passed, 0 failed and 838 were skipped.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

ftynse accepted this revision.Jan 29 2020, 2:51 PM

I'm fine with this, but please make sure @rriddle has no objections either.

mlir/include/mlir/Transforms/DialectConversion.h
324

I still see replaceUsesOfValue declared below, did you forget to upload something?

mlir/lib/Conversion/StandardToLLVM/ConvertStandardToLLVM.cpp
663

I erased it in my previous version but it was a bug. The placeholder must be alive since it goes into rewriter's mapping.

Even through the rewriter? (rewriter.erase calls rewriter.replaceOp internally)

We would need something like 'domInstFilter'

It looks like here we are in a more specific case, which seems to come back several times on the function boundary: we need to inject a new operation that takes the region argument, and replace all existing uses of the region argument with the new op. This sounds like this should be possible with the materializeConversion hook (https://github.com/llvm/llvm-project/blob/master/mlir/include/mlir/Transforms/DialectConversion.h#L135). I can take a look into that, but it's better to have this landed first.

This revision is now accepted and ready to land.Jan 29 2020, 2:51 PM

Thanks, much better!

mlir/lib/Conversion/StandardToLLVM/ConvertStandardToLLVM.cpp
2342

This API seems to still be an issue for extensibility. If you create the LLVMTypeConverter instead of customizing an existing one, how will this the next extension point be implemented/used?

rriddle accepted this revision.Jan 29 2020, 11:58 PM

It would be nice to see in a future revision if we can cleanup some of the extensibility aspects of the LLVM type converter.

mlir/include/mlir/Conversion/StandardToLLVM/ConvertStandardToLLVM.h
41

using the

54

-> /

132–146

These should really be using ///

mlir/lib/Conversion/StandardToLLVM/ConvertStandardToLLVM.cpp
63

It would be much better if we could use proper pass options for these instead:

https://mlir.llvm.org/docs/WritingAPass/#instance-specific-pass-options

76

Class/Function/Variable/etc. top-level comments should all be using ///

100

failed

107

Why not just return directly?

691

nit: Remove the newline

2342

+1. One direction that I intend to take TypeConverter is to allow registering additional callbacks for the type conversion similarly to how ConversionTarget is extensible. For these types of things, it is increasingly clear how clunky inheritance style modeling is.

dcaballe updated this revision to Diff 241615.Jan 30 2020, 5:20 PM
dcaballe marked 13 inline comments as done.

Address feedback

Thanks for the feedback, again! I addressed it and replied inline.
This is a summary of the pending items that I would suggest addressing separately:

  1. Utility to inject a new operation that takes the region argument, and replace all existing uses of the region argument with the new op (@ftynse).
  2. Extensibility aspects of the LLVM type converter (@rriddle, @dcaballe can help if needed).
  3. Use proper pass options for LLVMLoweringPass flags (@dcaballe).
  4. Extensibility aspects of populate* functions for LLVMLoweringPass (@dcaballe will bring this to discourse).
  5. Add support for CallOp to the bare pointer calling convention (TBD once #4 is addressed).

What do you think? Are you OK with this, @mehdi_amini?

mlir/include/mlir/Conversion/StandardToLLVM/ConvertStandardToLLVM.h
132–146

No strong opinion but just for my understanding... if I remember correctly, /// is for Doxygen purposes so it's only needed for public interfaces and members (?). That's why // is used for private members and .cpp documentation. Isn't that the case? At least that's what I've seen around.

LLVM Coding Standards seem a bit ambiguous here:
https://llvm.org/docs/CodingStandards.html#doxygen-use-in-documentation-comments

Don’t duplicate the documentation comment in the header file and in the implementation file. Put the documentation comments for public APIs into the header file. Documentation comments for private APIs can go to the implementation file. In any case, implementation files can include additional comments (not necessarily in Doxygen markup) to explain implementation details as needed.
mlir/include/mlir/Transforms/DialectConversion.h
324

Unused leftover, sorry.

mlir/lib/Conversion/StandardToLLVM/ConvertStandardToLLVM.cpp
63

I'm just trying to be consistent with the -use-alloca flag. I'll change this in a follow-up commit for both.

76

Same comment about Doxygen.

663

Even through the rewriter? (rewriter.erase calls rewriter.replaceOp internally)

Oh, that would have worked for the first version, yes! It should be OK now since we are replacing the placeholder (line 684). Thanks!

This sounds like this should be possible with the materializeConversion hook. I can take a look into that, but it's better to have this landed first.

It sounds good. Thanks!

2342

I totally agree with that direction! I couldn't see how to go much further in this commit without introducing significant changes in the conversion infra. I think that should happen in a separate commit. Are you OK with the current approach for this commit, @mehdi_amini? Any specific suggestion for this commit that doesn't require too many infra changes?

rriddle added inline comments.Jan 30 2020, 5:33 PM
mlir/include/mlir/Conversion/StandardToLLVM/ConvertStandardToLLVM.h
132–146

I prefer to keep comment style consistent to make the code base uniform, and less error-prone. Refactoring can often make things public that were private, and private that were public. By having a consistent style, there isn't a need to even think about any of that stuff. So for the sake of MLIR I've been trying to make sure that everyone is consistent.

dcaballe marked an inline comment as done.Jan 30 2020, 5:43 PM
dcaballe added inline comments.
mlir/include/mlir/Conversion/StandardToLLVM/ConvertStandardToLLVM.h
132–146

Ok, that's reasonable. I'll change that before committing. Thanks!

Unit tests: fail. 62357 tests passed, 1 failed and 839 were skipped.

failed: libc++.std/containers/sequences/array/array_creation/to_array.fail.cpp

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

mehdi_amini accepted this revision.Jan 30 2020, 7:23 PM
ftynse accepted this revision.Jan 31 2020, 12:32 AM

This is a summary of the pending items that I would suggest addressing separately:
<...>

Great, thanks for working on this! I've already started looking at possible solutions (https://reviews.llvm.org/D73702) on my side.

mlir/include/mlir/Conversion/StandardToLLVM/ConvertStandardToLLVM.h
132–146

Indeed, we have an (implicit) convention that top-level definitions and class members are commented with /// (although not all of the code base follows, yet). Let's document it in https://mlir.llvm.org/getting_started/DeveloperGuide/#style-guide, we already have some restrictions on top of LLVM's style.

ftynse marked an inline comment as done.Jan 31 2020, 12:39 AM
ftynse added inline comments.
mlir/include/mlir/Conversion/StandardToLLVM/ConvertStandardToLLVM.h
132–146

By the way, it wasn’t announced widely but : https://mlir.llvm.org/doxygen/

dcaballe updated this revision to Diff 241793.Jan 31 2020, 12:31 PM
dcaballe marked an inline comment as done.
  • Rebase to see if pre-commit test failure is gone
  • Fixed doxygen comments
  • Added -convert-loop-to-std flag to mlir-cpu-runner test. It failed without it after rebase.

Thank you all! Updating diff to see if the test failure is gone. I'll commit it in a few hours.

By the way, it wasn’t announced widely but : https://mlir.llvm.org/doxygen/

That's awesome! I use it a lot and had to be building it locally. Thanks!

mlir/include/mlir/Conversion/StandardToLLVM/ConvertStandardToLLVM.h
132–146

Great, thanks!

Unit tests: unknown.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

Unit tests: unknown.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

ftynse accepted this revision.Jan 31 2020, 2:52 PM

AFAIK, MLIR changes cannot affect libc++ tests so it's okay to land if those are the only problem.

Unit tests: pass. 62377 tests passed, 0 failed and 839 were skipped.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

This revision was automatically updated to reflect the committed changes.