This is an archive of the discontinued LLVM Phabricator instance.

[mlir][math] Added math::IPowI conversion to calls of outlined implementations.
ClosedPublic

Authored by vzakhari on Jul 14 2022, 3:26 PM.

Details

Summary

Power functions are implemented as linkonce_odr scalar functions
for integer types used by IPowI operations met in a module.
Vector form of IPowI is linearized into a sequence of calls
of the scalar functions.

Diff Detail

Event Timeline

vzakhari created this revision.Jul 14 2022, 3:26 PM
Herald added a project: Restricted Project. · View Herald Transcript
vzakhari requested review of this revision.Jul 14 2022, 3:26 PM
vzakhari updated this revision to Diff 445030.Jul 15 2022, 9:43 AM
vzakhari updated this revision to Diff 446941.Jul 22 2022, 12:30 PM
vzakhari retitled this revision from [mlir][math] Added math::IPowSI conversion to LLVM dialect. to [mlir][math] Added math::IPowI conversion to LLVM dialect..
vzakhari edited the summary of this revision. (Show Details)
vzakhari updated this revision to Diff 449430.Aug 2 2022, 2:03 PM

rebase

Please review.

Mogball requested changes to this revision.Aug 11 2022, 10:11 AM
Mogball added a subscriber: Mogball.
Mogball added inline comments.
mlir/include/mlir/Conversion/Passes.td
495

Please change this back to a generic pass.

mlir/lib/Conversion/MathToLLVM/MathToLLVM.cpp
284 ↗(On Diff #449430)

Lowering this to a function call is a little unusual. This lowering makes more sense to be a separate pass, because MathToLLVM is meant for trivial mostly one-to-one conversions

This revision now requires changes to proceed.Aug 11 2022, 10:11 AM
vzakhari added inline comments.Aug 11 2022, 10:15 AM
mlir/lib/Conversion/MathToLLVM/MathToLLVM.cpp
284 ↗(On Diff #449430)

Okay, this makes sense! I will create a pass like MathToOutlinedImpl - please suggest if you can think of a better name.

Is it possible to implement the function in arith or other math operations before lowing to LLVM? Probably MathExpandToFuncs

Is it possible to implement the function in arith or other math operations before lowing to LLVM? Probably MathExpandToFuncs

It should be possible. Let me try it.

vzakhari updated this revision to Diff 452842.Aug 15 2022, 4:25 PM
vzakhari retitled this revision from [mlir][math] Added math::IPowI conversion to LLVM dialect. to [mlir][math] Added math::IPowI conversion to calls of outlined implementations..
Mogball added inline comments.Aug 16 2022, 8:49 AM
mlir/include/mlir/Conversion/Passes.td
522
mlir/lib/Conversion/MathToFuncs/MathToFuncs.cpp
63–64

The type should only be omitted if it's explicit on the right hand side on an assignment due to a cast, or if it's "too long" (e.g. vector<int32_t>::iterator)

66–70

Can you add a failure reason with rewriter.notifyMatchFailure? It can notoriously difficult to debug pattern matching without those messages.

71

Spell out the auto.

72

Should this be unsigned?

79

Please don't use auto for a loop index.

90
130

Please spell out all the autos as appropriate.

140

Would generating this function be made any easier by using scf instead of cf?

328–330

Since this is a partial dialect conversion, can you not just add math::IPowI as an illegal op?

335–339

Nit: typically this comes before the pass declaration/definition

Mogball requested changes to this revision.Aug 16 2022, 8:51 AM
This revision now requires changes to proceed.Aug 16 2022, 8:51 AM
vzakhari added inline comments.Aug 16 2022, 10:19 AM
mlir/lib/Conversion/MathToFuncs/MathToFuncs.cpp
63–64

I will fix auto declarations. I will keep auto for results of rewriter.create<T> - it looks like it is a common practice to write it with auto.

66–70

Sure! Thanks for the suggestion!

72

int64_t should be correct, since ShapedType::getNumElements is defined as returning int64_t in IR/BuiltinAttributes.h.

140

It seems to me unstructured control flow fits better for the code with early returns from the function.

328–330

Will do.

vzakhari updated this revision to Diff 453059.Aug 16 2022, 10:20 AM

Friendly ping. Please review.

The high level idea makes sense to me, but I think the pass should be structured as:

  1. Collect all element types for which functions need to be instantiated and instantiate those functions
  2. Run the vector to scalar conversions and scalar to function call rewrites
mlir/include/mlir/Conversion/MathToFuncs/MathToFuncs.h
13

You can remove this include

24

These functions normally don't take a PatternBenefit.

27

These functions normally return just a Pass

mlir/include/mlir/Conversion/Passes.td
523

Why is this type of linkage needed? It's unfortunate that this pass depends on the LLVM dialect just for this.

mlir/lib/Conversion/MathToFuncs/MathToFuncs.cpp
125

It's generally not valid for patterns to access and modify parent operations, especially since these "patterns" are exposed publicly. The generation of these software implementations should not be done inside patterns. Patterns should be local rewrites of operations.

The high level idea makes sense to me, but I think the pass should be structured as:

  1. Collect all element types for which functions need to be instantiated and instantiate those functions
  2. Run the vector to scalar conversions and scalar to function call rewrites

Can you please explain the benefit of such a split? There are 4 existing conversion passes that access their getNearestSymbolTable() "on the fly" (ComplexToLibm, MathToLibm, MemRefToSPIRV and SCFToOpenMP), and I do not really want to invent the wheel here :)

Thank you for the review! I will upload updated files shortly.

mlir/include/mlir/Conversion/MathToFuncs/MathToFuncs.h
24

There are some (e.g. ComplexToLLVM, MathToLibm), but it is currently not needed so I will remove it. It may be used in future in case there is target/project specific handling for such operations, e.g. in some cases there is target/project specific conversion for some flavors of the operation that is more preferable than MathToFuncs conversion - as I understand, in this case the patterns may be selected based on their benefit.

27

I see that many-many passes that are defined as ModuleOp passes in mlir/lib/Conversion/Passes.td (i.e. they are derived from Pass<"...", "ModuleOp">) all return std::unique_ptr<OperationPass<ModuleOp>> in the CPP files in mlir/lib/Conversion/*/*.cpp. I am not comfortable with stepping away from the common pattern. I guess it should be written this way to make it clear that the pass is not a generic pass but rather a specific operation pass.

mlir/include/mlir/Conversion/Passes.td
523

In FuncToLLVM conversion all func.func operations have external linkage by default. We cannot use external linkage for the generated functions, because there may be multiple definitions in different translation modules and the linking will fail. So we have to specify either internal or linkonce_odr linkage - the latter is better, because it allows the linker to keep only one copy of the generated function across multiple modules being linked.

I do not know of any other way to control linkage in MLIR.

mlir/lib/Conversion/MathToFuncs/MathToFuncs.cpp
125

I understand the concern. I followed the same approach that MathToLibm and ComplexToLibm use.

I guess I can add a comment in MathToFuncs.h to warn about valid usage of populateMathToFuncsConversionPatterns - does it make sense?

vzakhari updated this revision to Diff 454126.Aug 19 2022, 3:38 PM

Addressed review remarks.

The benefit is that the patterns won't 1. Keep looking up functions in the symbol table, which is painfully slow and 2. Won't violate the API contract for patterns.

mlir/include/mlir/Conversion/MathToFuncs/MathToFuncs.h
24

In that case, they wouldn't use these patterns or this pass. The validity of composing patterns is inconsistent.

27

It is inconsistent throughout the MLIR codebase, but please return a Pass. There is no benefit to returning OperationPass<ModuleOp> because the pass manager API only accepts the former anyways.

mlir/include/mlir/Conversion/Passes.td
523

Adding linkage to FuncDialect appears to be a TODO. It's fine for now then.

mlir/lib/Conversion/MathToFuncs/MathToFuncs.cpp
125

No. Those other patterns are also invalid. Patterns should not be modifying parent operations. It's something that "just happens to work" because no one has needed to compose these patterns sets in a parallelized pass (which would result in race conditions). This is especially true in conversion patterns, because there is extra bookkeeping in the dialect conversion pass that is violated if this happens.

Please don't propagate what has been done in the older passes and separate these.

vzakhari added inline comments.Aug 22 2022, 5:51 PM
mlir/include/mlir/Conversion/MathToFuncs/MathToFuncs.h
27

Will do.

mlir/lib/Conversion/MathToFuncs/MathToFuncs.cpp
125

Thank you for the explanation!

Then it looks like exposing the patterns is unsafe, in general, and I should rewrite the pass as you suggested without externalizing the match patterns (populateMathToFuncsConversionPatterns). The module pass will first visit all IPowI operations for collecting information about the kinds of implementation functions that need to be created in the module, then it will create the new functions, and then it will apply populateMathToFuncsConversionPatterns patterns to rewrite the operations into the calls.

vzakhari updated this revision to Diff 455409.Aug 24 2022, 3:59 PM

Addressed review comments.

Good evolution! The direction of the patch LGTM. I just have some minor complaints about the implementation details.

mlir/include/mlir/Conversion/MathToFuncs/MathToFuncs.h
14–16
mlir/include/mlir/Conversion/Passes.td
519
522–523
mlir/lib/Conversion/MathToFuncs/MathToFuncs.cpp
9–25

Can you trim these includes? I don't think all of these are needed

30–37

This isn't necessary. FunctionType can be hashed directly in a DenseMap.

153

I would prefer this to be a standalone function, not a static function on the pattern.

340–343

These two checks aren't necessary since they are invariants enforced by the verifier. You just need to check that this is a scalar integer op (done by the first condition).

376
383–384
385–387

If all the types are the same, then you only need to hash based on the element type.

Do you also mind marking resolved comments as "done"?

vzakhari marked 27 inline comments as done.Aug 24 2022, 6:16 PM

Thank you for the prompt review!

mlir/lib/Conversion/MathToFuncs/MathToFuncs.cpp
383–384

Since I am going to add FPowI support later, does it make sense to keep the switch now?

385–387

This is true for IPowI but not for FPowI. I prefer to keep it as a function type so that the keys are consistent in the map, otherwise, for IPowI we will have IntegerType keys and for FPowI we will have FunctionType keys.

vzakhari updated this revision to Diff 455451.Aug 24 2022, 6:17 PM

Do you also mind marking resolved comments as "done"?

Done :)

Mogball added inline comments.Aug 25 2022, 9:24 AM
mlir/lib/Conversion/MathToFuncs/MathToFuncs.cpp
383–384

Yes it does. Thanks.

385–387

Then you can just use Type as the key.

385–387

In any case, you don't need std::map and a custom comparator. DenseMap<Type, T> works just fine.

vzakhari updated this revision to Diff 455726.Aug 25 2022, 2:59 PM
vzakhari marked 3 inline comments as done.
vzakhari added inline comments.
mlir/lib/Conversion/MathToFuncs/MathToFuncs.cpp
385–387

Should be done now. Thanks!

Mogball accepted this revision.Aug 25 2022, 3:16 PM

LGTM. Just one last complaint about the stringify/hashing

mlir/lib/Conversion/MathToFuncs/MathToFuncs.cpp
40

We generally don't use std::function

62
66

I think the "function type stringify" is too generic. For FPowI, just the floating point and integer type will suffice. This doesn't need to be done generally. I think you can just remove this function.

152–153
362–376

Can you remove the extra logic for now? I know you want to add support for other ops, but I think it'd be less automagical to just add another Case for FPowI

This revision is now accepted and ready to land.Aug 25 2022, 3:16 PM
vzakhari added inline comments.Aug 25 2022, 4:19 PM
mlir/lib/Conversion/MathToFuncs/MathToFuncs.cpp
66

Okay, I will remove it.

152–153

Will do. flush should not be necessary according to this: https://llvm.org/doxygen/classllvm_1_1raw__string__ostream.html#details

vzakhari updated this revision to Diff 455751.Aug 25 2022, 4:20 PM

Please let me know if everything looks good now. Thank you for all the feedback!

Mogball accepted this revision.Aug 25 2022, 4:21 PM
vzakhari marked 5 inline comments as done.Aug 25 2022, 4:21 PM