This is an archive of the discontinued LLVM Phabricator instance.

Lower F08 bitwise-reduction intrinsics (IALL, IANY, IPARITY)
ClosedPublic

Authored by tarunprabhu on Jul 12 2022, 8:31 PM.

Details

Summary

This patch implements lowering for F08's IALL, IANY and IPARITY intrinsics. This calls the corresponding runtime functions. The implementation follows the implementation of the SUM and PRODUCT intrinsics.

Diff Detail

Event Timeline

tarunprabhu created this revision.Jul 12 2022, 8:31 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 12 2022, 8:31 PM
tarunprabhu requested review of this revision.Jul 12 2022, 8:31 PM

Thanks a lot for plugging all that runtime. Apart from the KIND 16 issue in the fir::runtime::genXXX, this looks great to me.

flang/lib/Optimizer/Builder/Runtime/Reduction.cpp
1027

Currently, the 128 bits case need to be manually defined because the runtime declaration are not accessible on all hosts (see the windows bot failure). See ForcedMinvalInteger16 for an example of how to manually define it.

The lowering functions for parity, iall and iany seem very similar. Is it possible to share some code?

The lowering functions for parity, iall and iany seem very similar. Is it possible to share some code?

Yes, it should be possible. mkRTKey seems to involve token pasting, so I'm not sure clean the result will be. I'll work on an update.

Handle the kind=16 case by manually generating the call to the runtime function.

Since the code for iall, iany and iparity were the same, the whole code was turned into a macro. I couldn't think of a cleaner way of doing it since mkRTKey was also a macro and relied on token pasting.

Added more tests, particularly for the kind=16 case.

peixin added a subscriber: peixin.Sep 16 2022, 2:39 AM

No test cases?

Can the runtime support the following special case?
IALL: If ARRAY has size zero the result value is equal to NOT (INT (0, KIND (ARRAY))).

flang/lib/Lower/IntrinsicCall.cpp
811

Should dim also be handleDynamicOptional to support the case IALL (ARRAY [, MASK])? Same for iany.

flang/lib/Optimizer/Builder/Runtime/Reduction.cpp
377

This is not consistent with others. Should #define EXPAND_AND_QUOTE(S) ExpandAndQuoteKey(RTNAME(S)) be moved to where ExpandAndQuoteKey is defined?

tarunprabhu marked an inline comment as done.Sep 16 2022, 6:39 AM

Thanks @tarunprabhu for addressing the previous comments, I agree with @peixin that some lit tests should be added in flang/test/Lower/Intrinsics and that it would make sense to move the EXPAND_AND_QUOTE macro definition in flang/Optimizer/Builder/Runtime/RTBuilder.h.

Otherwise LGTM.

flang/lib/Lower/IntrinsicCall.cpp
811

No. Optional intrinsic arguments are tricky. There is a distinction between defining SOME_INTRINSIC signature being SOME_INTRINSIC(ARG) and SOME_INTRINSIC() vs SOME_INTRINSIC([ARG]).

In the first case, ARG is either syntactically present or absent in the source, and when it is syntactically present it is not an OPTIONAL argument: its actual argument cannot be an absent optional dummy, a disassociated pointer, nor an unallocated allocatable. The actual cannot be "dynamically" absent.

In the second case [ARG] is an OPTIONAL argument in the Fortran sense (the argument description in the standard states '(optional)', see MASK for instance. This is not the case for DIM). Its actual argument can be an actual argument cannot be an absent optional dummy, a disassociated pointer, or an unallocated allocatable. handleDynamicOptional is meant to deal with these cases, so it would add a pointless overhead to use it for DIM here.

peixin added inline comments.Sep 19 2022, 1:49 AM
flang/lib/Lower/IntrinsicCall.cpp
811

Got it. Thanks.

Renamed the EXPAND_AND_QUOTE macro to EXPAND_AND_QUOTE_KEY and moved it to RTBuilder.h

Thanks @tarunprabhu for addressing the previous comments, I agree with @peixin that some lit tests should be added in flang/test/Lower/Intrinsics and that it would make sense to move the EXPAND_AND_QUOTE macro definition in flang/Optimizer/Builder/Runtime/RTBuilder.h.

Otherwise LGTM.

No test cases?

Bah! Sorry, my mistake. I didn't check the diff properly. The tests were there, but did not get included in the diff.

Can the runtime support the following special case?
IALL: If ARRAY has size zero the result value is equal to NOT (INT (0, KIND (ARRAY))).

I just ran a simple test with the -flang-experimental-exec flag and it seems to give the expected result. However, I am not familiar with the runtime, so someone else will have to give you a definitive answer, I'm afraid.

tarunprabhu marked 4 inline comments as done.Sep 21 2022, 9:18 AM
This revision is now accepted and ready to land.Sep 22 2022, 4:36 AM
peixin accepted this revision.Sep 22 2022, 5:01 AM

LGTM