In a future patch we plan on introducing a large set of Power-PC specific intrinsics. During our prototyping we found that the number of function type generators we were defining, plus those already defined, were reaching an unreasonable number. This patch introduces a generic function type generator function that can be used for almost all cases. The generator supports creating function types with up to 4 arguments and with arguments/return type of types: void, integer, real, and comlex. The intention is for a future patch, which introduces a set of PowerPC-specific vector intrinsics, to also introduce support in the generator for: integer vector, unsigned vector, and real vector types.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
flang/include/flang/Optimizer/Builder/IntrinsicCall.h | ||
---|---|---|
503 | It is probably worth asserting the supported widths. | |
flang/lib/Optimizer/Builder/IntrinsicCall.cpp | ||
888 | The element size is not 24 bits in this case. To avoid such weird cases, can we use the kind as the template argument instead of the bit-width? Then you may use FirOpBuilder::getRealType and KindMapping::getIntegerBitsize for mapping the kind to the bitsize in genFuncType. |
flang/include/flang/Optimizer/Builder/IntrinsicCall.h | ||
---|---|---|
487 | Could you turn this also in a fatal error? In release mode, the assert does not help. |
flang/lib/Optimizer/Builder/IntrinsicCall.cpp | ||
---|---|---|
888 | Done, genFuncType now uses kind instead of bitwidth. I had to override the type declaration for FuncTypeBuilderFunc to include an additional FirOpBuilder argument so that the builder can be used for the kind-to-bitwidth calculations. I made this type declaration inside of MathOperations to limit the scope, any references to FuncTypeBuilderFunc outside of IntrinsicCall still expects a function type that only has an MLIRContext argument |
flang/include/flang/Optimizer/Builder/IntrinsicCall.h | ||
---|---|---|
509–520 | I'm not against current approach, but I want to suggest an alternative, if others agree. If the type enum and kind could be encapsulated in a new type, then it would be possible to use a variadic template here, to support an arbitrary number of arguments. It could be something similar to the edit above. |
flang/include/flang/Optimizer/Builder/IntrinsicCall.h | ||
---|---|---|
509–520 | I am a fan of this approach and appreciate the code your provided. I will try to switch to this approach and update the patch when successful. Thanks! |
flang/include/flang/Optimizer/Builder/IntrinsicCall.h | ||
---|---|---|
509–520 | static_assert(t != ParamTypeId::Integer || k == 4 || k == 8, "Unsupported integer kind"); static_assert(t != ParamTypeId::Real || k == 4 || k == 8, "Unsupported real kind"); This new code now checks supported kinds as an assert in addition to the llvm::report_fatal_errors in getTypeHelper (when trying to convert to mlir::Type). Having both seems like overkill, do we prefer to have the static_asserts or the report_fatal_errors for the supported kind checks |
flang/include/flang/Optimizer/Builder/IntrinsicCall.h | ||
---|---|---|
509–520 | As the supported kinds can now be checked at compile time, I think it would be better to remove the llvm::report_fatal_errors. |
Ping.
Now using variadic arguments and opted for static_assert over report_fatal_error where appropriate since its asserted at compile time. Thanks for the suggestion @luporl
Thanks for replacing all those function type generators with a generic one!
LGTM. I have just a few nitpicks.
You might want to wait a little longer to see if anyone has more comments on this patch.
flang/include/flang/Optimizer/Builder/IntrinsicCall.h | ||
---|---|---|
485 | Unsupported complex kind | |
516 | typo: bitsize | |
539–540 | Is this if still needed? |
Addressed nits
flang/include/flang/Optimizer/Builder/IntrinsicCall.h | ||
---|---|---|
539–540 | Because Void arguments are no longer expect (as a result of the variadic args), and because getTypeHelper will report fatal_error if it recieves void, this if statement was no longer needed. Thanks |
Patch is approved but I will wait a bit before landing in case there are additional nits
But I shouldn't wait for too long as we have other patches dependent on and waiting for this to land first
Unsupported complex kind