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.
Details
Diff Detail
Event Timeline
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 |
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
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 |
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. |
The high level idea makes sense to me, but I think the pass should be structured as:
- Collect all element types for which functions need to be instantiated and instantiate those functions
- 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. |
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? |
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. |
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. |
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. |
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. |
mlir/lib/Conversion/MathToFuncs/MathToFuncs.cpp | ||
---|---|---|
385–387 | Should be done now. Thanks! |
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 |
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 |
You can remove this include