This is an archive of the discontinued LLVM Phabricator instance.

[Flang] Replace intrinsic function type generators with single generic function type generator
ClosedPublic

Authored by pscoro on May 31 2023, 8:35 AM.

Details

Summary

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.

Diff Detail

Event Timeline

pscoro created this revision.May 31 2023, 8:35 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMay 31 2023, 8:35 AM
pscoro requested review of this revision.May 31 2023, 8:35 AM
pscoro updated this revision to Diff 527064.May 31 2023, 8:37 AM

Removed outdated comment

pscoro edited the summary of this revision. (Show Details)May 31 2023, 8:42 AM
vzakhari added inline comments.Jun 2 2023, 6:29 PM
flang/include/flang/Optimizer/Builder/IntrinsicCall.h
507

It is probably worth asserting the supported widths.

flang/lib/Optimizer/Builder/IntrinsicCall.cpp
880

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.

tschuett added inline comments.
flang/include/flang/Optimizer/Builder/IntrinsicCall.h
491

Could you turn this also in a fatal error? In release mode, the assert does not help.

pscoro updated this revision to Diff 528468.Jun 5 2023, 9:03 AM

Addressed comments, genFuncType now uses kind instead of bit width

pscoro marked an inline comment as done.Jun 5 2023, 9:09 AM
pscoro added inline comments.
flang/lib/Optimizer/Builder/IntrinsicCall.cpp
880

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

pscoro marked an inline comment as done.Jun 5 2023, 9:42 AM
luporl added a subscriber: luporl.Jun 6 2023, 7:52 AM
luporl added inline comments.
flang/include/flang/Optimizer/Builder/IntrinsicCall.h
513–524

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.

pscoro added inline comments.Jun 6 2023, 8:08 AM
flang/include/flang/Optimizer/Builder/IntrinsicCall.h
513–524

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!

pscoro added inline comments.Jun 6 2023, 8:47 AM
flang/include/flang/Optimizer/Builder/IntrinsicCall.h
513–524
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

luporl added inline comments.Jun 6 2023, 9:47 AM
flang/include/flang/Optimizer/Builder/IntrinsicCall.h
513–524

As the supported kinds can now be checked at compile time, I think it would be better to remove the llvm::report_fatal_errors.

pscoro added inline comments.Jun 6 2023, 10:56 AM
flang/include/flang/Optimizer/Builder/IntrinsicCall.h
513–524

Do static_asserts get checked in release builds? I know asserts are typically debug only.

I am also hesitating because of @tschuett 's previous comment that requested using fatal_error over asserts

No worries. static_assert is a compile-time assert.

pscoro updated this revision to Diff 528976.Jun 6 2023, 12:01 PM

Implemented variadic template arg suggestion

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

luporl accepted this revision.Jun 6 2023, 1:45 PM

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
489

Unsupported complex kind

520

typo: bitsize

543–544

Is this if still needed?

This revision is now accepted and ready to land.Jun 6 2023, 1:45 PM
tschuett added inline comments.Jun 7 2023, 12:52 AM
flang/include/flang/Optimizer/Builder/IntrinsicCall.h
542

Nit: ++i

551

The else is kind of redundant. The If will return.

pscoro updated this revision to Diff 529295.Jun 7 2023, 7:34 AM
pscoro marked 9 inline comments as done.

Addressed nits

flang/include/flang/Optimizer/Builder/IntrinsicCall.h
543–544

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

pscoro added a comment.Jun 7 2023, 7:36 AM

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