Page MenuHomePhabricator

[flang] lower sum intrinsic to hlfir.sum operation

Authored by tblah on Jan 30 2023, 7:43 AM.

Diff Detail

Event Timeline

tblah created this revision.Jan 30 2023, 7:43 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJan 30 2023, 7:43 AM
tblah requested review of this revision.Jan 30 2023, 7:43 AM
clementval added inline comments.Jan 31 2023, 12:56 AM

We probably want to have a more generic way to lower those intrinsic operations. Something like the table in flang/lib/Lower/IntrinsicCall.cpp.

Nits about the builder and extendedValueToHlfirEntity usage. Looks good otherwise.


fir::ExtendedValue should not contain hlfir::ExprType (all the helpers and functions only expect to work on fir::ExtendedValue that wraps fir/mlir types).


When lowering fully switch to the new expression representation, it will be possible to do so I think, but right now, it is not really possible because IntrinsicCall.cpp is based on the fir::ExtendedValue representation which requires array operands to be in memory, and the whole point of HLFIR is to try to avoid creating the temporary (to directly inline SUM(A+B) without ever creating a temp for A+B).

So I am OK with this not very optimal approach to start with, then we can improve this when we do not have to jungle between both lowering.


sumOp is not a fir::ExtendedValue. hlfir.epxr already are valid HLFIR entities, you can directly return hlfir::EntityWithAttributes{sumOp.getResult()}; (that will only assert that sumOp.getResult() is indeed a valid HLFIR entity type).


You could have used numType = hlfir::getFortranElementType(array.getType()) here, but I do not think you even need it if you always use the element type from stmtResultType (and since the assert should not be added here for properties that are verified in the verifier).


Better to not assert here since the verifier deals with this.


Is there a case where stmtResultType would be absent ? I think it would be simpler to require it to be present.


Same here.


Same here.

jeanPerier added inline comments.Feb 8 2023, 6:01 AM

Here, I think it would be best to add dim = dim ? hlfir::loadTrivialScalar(loc, builder, dim) as well as array = hlfir::derefPointersAndAllocatables(loc, builder, array) so that the operands of the hlfir.sum are decorelated from the dereferences as much as possible.

The mask case is a bit trickier since it may be dynamically optional. hlfir.sum may need an extra optional i1 value argument to hold the dynamic presence of the mask in this case, but it is also ok to call derefPointersAndAllocatables on deallocated pointers/allocatable: the result is a that contains a null address.

tblah updated this revision to Diff 495836.Feb 8 2023, 7:09 AM
tblah marked 8 inline comments as done.

Thanks for reviewing

  • Respond to review feedback
  • Add tests

See discussion on the first patch in the series. This omission is deliberate because I think it will make it simpler for passes to pattern match on this if we don't convert the operands until lowering to the runtime call.

For example, a pass might want to match MATMUL(TRANSPOSE(...), ....). This is simpler if the the generated HLFIR is something like

%transpose_expr = hlfir.transpose [...] : hlfir.expr<[...]>
%other_expr = [...] : hlfir.expr<[...]>
%mul_expr = hlfir.matmul %transpose_expr %other_expr

But I am open to your input on this

tblah retitled this revision from WIP: [flang] lower sum intrinsic to hlfir.sum operation to [flang] lower sum intrinsic to hlfir.sum operation.Feb 8 2023, 7:10 AM
tblah edited the summary of this revision. (Show Details)
jeanPerier added inline comments.Feb 8 2023, 7:54 AM

Your example makes absolute sense to me, but I think pattern matching will not be impacted by loads of scalars when obvious (like here with DIM), or dereference of allocatable and pointers.
I am inclined to think having a single place where this special cases are handled is simpler than in the operation transform that are likely to not be done with the allocatable/pointer case in mind.

For instance, would the current hlfir.sum handle:

subroutine foo(s, x, dim)
  integer, pointer :: dim
  real :: x(:, :), s(:)
  s = sum(x, dim)
end subroutine

It is a stupid edge case that is unlikely to be found in real apps, and, if an issue, would only be discovered quite late.

Using derefPointersAndAllocatables/loadTrivialScalar when possible does not affect the chaining of transformational since it does not touch values (hlfir.expr and simple scalars, which should be what is returned by sub-expression lowered nodes) and in my opinion will allow for simpler and more robust transformational operations.

I understand however that is is a bit inconvenient/not so nice to have to "split" the argument cooking between lowering and the pass that translate the operation to HLFIR.

tblah updated this revision to Diff 496087.Feb 9 2023, 4:31 AM
tblah marked 2 inline comments as done.


  • Add load operation for pass-by-ref DIM argument
  • derefPointersAndAllocatables on ARRAY argument
  • Add a test for pass-by-pointer DIM argument
jeanPerier accepted this revision.Feb 10 2023, 12:47 AM

Small nits. looks good otherwise!
Please merge at the same time of after the codegen patch (D143512) so that this patch does not break the end-to-end flow.


It may be better to place this code in a helper lambda and call it when needed (only for SUM here), rather than doing this for all other intrinsics where "operands" will not be used.


Could you add a static llvm::cl::opt<bool> to control the intrinsic lowering to use the hlfir ops or not ?

I am OK with having it on by default, but I think it will be interesting to be able to run some test suits/benchmark with it on/off to catch potential issues.


If you want to assert, I think it would be most useful on operands[0] since the line above would crash if it is null (hlfir::Entity cannot be created from null values).

This revision is now accepted and ready to land.Feb 10 2023, 12:47 AM
tblah updated this revision to Diff 496422.Feb 10 2023, 4:09 AM
tblah marked 3 inline comments as done.

Fix nits in review

This revision was landed with ongoing or failed builds.Feb 13 2023, 2:52 AM
This revision was automatically updated to reflect the committed changes.