[flang-commits] [PATCH] D129616: Lower F08 bitwise-reduction intrinsics (IALL, IANY, IPARITY)

[Jean Perier via Phabricator via flang-commits]

jeanPerier added a comment.

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.



================
Comment at: flang/lib/Lower/IntrinsicCall.cpp:811
+     {{{"array", asBox},
+       {"dim", asValue},
+       {"mask", asBox, handleDynamicOptional}}},
----------------
peixin wrote:
> Should `dim` also be `handleDynamicOptional` to support the case `IALL (ARRAY [, MASK])`? Same for `iany`.
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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129616/new/

https://reviews.llvm.org/D129616