This is an archive of the discontinued LLVM Phabricator instance.

[flang] add hlfir.sum operation
ClosedPublic

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

Details

Summary

Add an HLFIR operation for the SUM transformational intrinsic, according
to the design set out in flang/doc/HighLevelFIR.md.

I decided to make hlfir.sum lenient about the form of its
arguments. This allows the sum intrinsic to be lowered to only this HLFIR
operation, without needing several operations to convert and box
arguments. Having only one operation generated for the intrinsic
invocation should make optimization passes on HLFIR simpler.
However, the DIM argument will be loaded into memory (not allowing
passing pointers or references).

Diff Detail

Event Timeline

tblah created this revision.Jan 30 2023, 7:37 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
tblah requested review of this revision.Jan 30 2023, 7:37 AM
clementval added inline comments.Jan 31 2023, 12:52 AM
flang/lib/Optimizer/HLFIR/IR/HLFIRDialect.cpp
109

You don't consider fir.heap and fir.ptr types so it is likely to not work properly with POINTER and ALLOCATABLE arrays/descriptor

110
113
flang/lib/Optimizer/HLFIR/IR/HLFIROps.cpp
432

To support polymorphic descriptor as well.

452

spell out auto here.

455

spell out auto

tblah updated this revision to Diff 493556.Jan 31 2023, 3:48 AM

Thanks for the review.

Changes:

  • Support fir.heap and fir.ptr types and add tests for this
  • Support fir::BaseBoxType
  • Spell out auto types
tblah marked 6 inline comments as done.Jan 31 2023, 3:49 AM
tblah updated this revision to Diff 493587.Jan 31 2023, 6:28 AM

Changes: add accessors SumOp::getDim and SumOp::getMask

Thanks a lot for working on this!

flang/include/flang/Optimizer/HLFIR/HLFIROpBase.td
79

I do not see any uses, do you plan using it?

flang/include/flang/Optimizer/HLFIR/HLFIROps.td
269

That is a bit hard to read, and could maybe re-use some existing logic.

I think it would be simpler to check AnyFortranNumericalArray and to move logic (https://github.com/llvm/llvm-project/blob/main/flang/include/flang/Optimizer/Builder/HLFIRTools.h#L51).

That could be implemented as: isFortranScalarNumericalType(getFortranElementType(type)) && !isBoxAddressType(type);. The last condition excludes "non dereferenced" pointer and allocatable
I do not think transformational should accept them since it adds side effects related to the pointer/allocatable dereference that are probably better kept in a fir.load.

I am missing a short word in "AnyFortranNumericalArray" to express this (NonAllocatableNorPointer is a bit heavy), maybe we could as a convention terminates the predicate that refuses fir.ref<fir.box<>> as "Object" ("AnyFortranNumericalArrayObject").

270

I personally think it would be clearer and simpler to use two Optional<...> here rather than to rely on the type. It makes the story simpler and then, you can use Optional<AnyIntegerType> for DIM and Optional<AnyFortranBooleanObject> for the mask. And you would not need custom accessors.
Under the hood, the MLIR operation is already a vector of mlir::Value anyway.
You will likely need to add the AttrSizedOperandSegments given there is more than one optional argument.

You may also need a fourth Optional<I1>:$mask_is_present to deal with the cases where the mask may be dynamically present/absent (deallocated allocatable/pointer or optional dummy variable). So far, in HLFIR, it should be avoided to assume it will be possible to find if a value is optional or not by looking at its defining operation because it may not be visible . I am starting to think this may need to be changed and to add something at the type level, but for now, it seems safer to me to indicate if an operation needs special care about an argument (and might be harder to optimize because of that).

flang/lib/Optimizer/HLFIR/IR/HLFIROps.cpp
496

You should be able to use hlfir::getFortranElementOrSequenceType(array.getType()) that will always return a fir::SequenceType (it translates hlfir::ExprType to it). You can then do

auto arrayTy = hlfir::getFortranElementOrSequenceType(array.getType()).dyn_cast<fir::SequenceType>()
if (!arrayTy)
  return emitOpError("ARRAY argument must be an array");
llvm::ArrayRef<int_64_t> arrayShape = arrayTy.getShape();

You should then get rid of unwrapType and getArrayOrExprTypeShape.

When you need a scalar type from a type (removing all fir.ptr/fir.heap...fir.box...fir.sequence, or hlfir.expr wrapping types), you can use hlfir::getFortranElementType.

504

Inside MLIR builder/verifier, do not assert here unless you the code below would crash if this was wrong. The MLIR verification error will give better feedback than an assert, and the assert will prevent the compiler to gracefully stop.

530

Making this an error might, in rare cases cause false positive: if for any reason SUM is called in an unreachable region (say if (confaormable(...)) then x = sum(....)), then it would be wrong to make it an error. This is rather unlikely (I think) that we get this kind of code with arrays that have compile time constant extents (much more likely with assumed shape, and we could imagine updating SUM after inlining one day).

I think emitWarning might be better as long as hlfir.sum code generation does not crash if this predicate is not true (but it is OK to generate "invalid code" since a warning was emitted and this code is invalid to start with and should never be reached).

vzakhari added inline comments.
flang/include/flang/Optimizer/HLFIR/HLFIROps.td
261

This may be done separately, but this will need to support ArithFastMathInterface.

tschuett added inline comments.
flang/lib/Optimizer/HLFIR/IR/HLFIROps.cpp
449

This looks like std::optional<mlir::Value> tryGetMask().

tblah updated this revision to Diff 495596.EditedFeb 7 2023, 10:39 AM
tblah marked 7 inline comments as done.

Thanks for review everyone.

Changes:

  • Moved type predicates to HLFIRTools from HLFIRDialect
  • Use getFortranElementType() helper
  • Renamed IsFortran\*BoxRefArrayOrExprType\* to IsFortran\*ArrayObject\*
  • Added ArithFastMathInterface
  • Removed varadic arguments, replacing them with two optional arguments
  • Modified assembly format to label each optional argument e.g. hlfir.sum %array dim %dim mask %mask
  • Added missing tests
tblah retitled this revision from WIP: [flang] add hlfir.sum operation to [flang] add hlfir.sum operation.Feb 7 2023, 10:39 AM
tblah edited the summary of this revision. (Show Details)
tblah added inline comments.Feb 7 2023, 10:41 AM
flang/include/flang/Optimizer/HLFIR/HLFIROpBase.td
79

Well spotted, thanks!

tblah retitled this revision from [flang] add hlfir.sum operation to WIP: [flang] add hlfir.sum operation.Feb 7 2023, 10:42 AM
tblah edited the summary of this revision. (Show Details)

Some comments on the type predicate. I think the patch is missing the D143503 dependency to apply correctly.
Looks good otherwise!

flang/include/flang/Optimizer/Builder/HLFIRTools.h
367

In lowering so far I did not allow the generation of array expression of i1 type (hlfir.expr<?xi1>) (but did not had it to the hlfir.expr verifier, maybe I should). The rational is that it is fine to handle scalar i1, but array of i1 are a bit problematic after bufferization because arrays are placed in memory, and placing i1 in memory would lead to an extra array temp overhead whenever this array needs to be passed to the runtime or another function (since it would need to be converted to an array with logical types).

flang/include/flang/Optimizer/HLFIR/HLFIROps.td
272

MASK can be a scalar in SUM (not a fascinating use case I admit, but legal). Do you plan to deal with this case in another way?

flang/lib/Optimizer/Builder/HLFIRTools.cpp
891

It looks like this logic will allow for both scalars and arrays. If that's what you want, then I think Array should be removed from the name, otherwise the logic could be:

if (isBoxAddressType(type))
  return false;
if (auto arrayTy = getFortranElementOrSequenceType(type).dyn_cast<fir::SequenceType>())
  return isFortranScalarNumericalType(arrayTy.getEleTy());
return false;
902

Same here. If you are in a template mood, you can even share the core logic:

template<bool (*SomeScalarTypePredicate)(mlir::Type)>
static bool isArrayObjectAndScalarPredicate(mlir::Type) {
  if (hlfir::isBoxAddressType(type))
    return false;
  if (auto arrayTy = hlfir::getFortranElementOrSequenceType(type).dyn_cast<fir::SequenceType>())
    return SomeScalarTypePredicate(arrayTy.getEleTy());
  return false;
}
hlfir::isFortranNumericalArrayObject(mlir::Type type) {
 return isArrayObjectAndScalarPredicate<isFortranScalarNumericalType>(type);
}

hlfir::isFortranLogicalOrI1ArrayObject(mlir::Type type) {
 return isArrayObjectAndScalarPredicate<isLogicalOrI1Type>(type);
}

Although in the case of MASK, I suspect scalar are OK, so your logic is good and the name needs to be changed.

jeanPerier added inline comments.Feb 8 2023, 12:45 AM
flang/include/flang/Optimizer/Builder/HLFIRTools.h
367

However, for scalars, i1 should be OK, so feel free to ignore that one if you change the predicate to allow scalars.

tblah updated this revision to Diff 495784.Feb 8 2023, 2:59 AM
tblah marked 6 inline comments as done.

Changes:

  • Rename isFortranLogicalOrI1ArrayObject to isMaskArgument
  • Don't allow arrays of i1 for the mask argument
  • Check that the array argument is an array in the tblgen type predicate rather than in SumOp::verify
flang/include/flang/Optimizer/Builder/HLFIRTools.h
367

Yes the intention is to allow i1 scalars. There are tests showing using scalars with this arg.

But that's a good point about arrays of i1. For now I will only allow arrays of logical.

flang/include/flang/Optimizer/HLFIR/HLFIROps.td
270

It seems intrinsic dynamically optional arguments are TODO:
https://github.com/llvm/llvm-project/blob/b83caa32dc86cfc4d7f7b160284c2f7d002dea75/flang/lib/Lower/ConvertCall.cpp#L1093

With this TODO, it is hard for me to test if I need the 4th argument. The lowering in FIR doesn't seem to do anything special here for SUM optional arguments. I'll come back to this once HLFIR supports dynamically optional intrinsic arguments.

flang/lib/Optimizer/Builder/HLFIRTools.cpp
891

Thanks, I should have done this in the first place - it will let me simplify SumOp::verify

tblah retitled this revision from WIP: [flang] add hlfir.sum operation to [flang] add hlfir.sum operation.Feb 8 2023, 2:59 AM
tblah edited the summary of this revision. (Show Details)
tblah updated this revision to Diff 495812.Feb 8 2023, 5:37 AM

Changes: allow the DIM argument to be passed by reference

Changes: allow the DIM argument to be passed by reference

What is the motivation to do this ? Given DIM cannot be dynamically optional and is simple integer scalar, I think it would be easier to just call loadTrivialScalar in lowering rather than to have to deal with DIM both in memory and value when doing hlfir.sum transformations. The simpler the op interface, the safer the rewrites/pattern match will be.

flang/include/flang/Optimizer/HLFIR/HLFIROps.td
271

I would keep the requirement

jeanPerier added inline comments.Feb 8 2023, 6:05 AM
flang/include/flang/Optimizer/HLFIR/HLFIROps.td
270

Agree this can be revisited later. I will get to this TODO at some point.

tblah added a comment.EditedFeb 8 2023, 6:05 AM

Thanks for taking another look!

Changes: allow the DIM argument to be passed by reference

What is the motivation to do this ? Given DIM cannot be dynamically optional and is simple integer scalar, I think it would be easier to just call loadTrivialScalar in lowering rather than to have to deal with DIM both in memory and value when doing hlfir.sum transformations. The simpler the op interface, the safer the rewrites/pattern match will be.

For something like this:

subroutine sum_dim(a, s, d)
  integer :: a(:,:), s(:), d
  s = SUM(a, d)
end subroutine

Here d is a !fir.ref<i32>. As I say in the patch description, I have aimed to be as permissive as possible with the arguments (only generating conversion operations when lowering to FIR) so that a hlfir.sum is just one operation which is easy to pattern match and move. In other words, I want this to lower without the extra fir.load:

func.func @sum_dim(%arg0 : !fir.box<!fir.array<?x?xi32>>, %arg1: !fir.box<!fir.array<?xi32>>, %arg2: !fir.ref<i32>) {
  %array = hlfir.declare %arg0 [...]
  %ret = hlfir.declare %arg1 [...]
  %dim = hlfir.declare %arg2 [...]
  %expr = hlfir.sum %array#0 dim %dim#0 : ([...]) -> !hlfir.expr<?xi32>
  hlfir.assign %expr to %ret#0 [...]
  hlfir.destroy %expr [...]
  return
}
tblah updated this revision to Diff 496086.Feb 9 2023, 4:29 AM
tblah marked an inline comment as done.

Changed to disallow pass-by-ref dim argument

tblah edited the summary of this revision. (Show Details)Feb 9 2023, 5:10 AM

Thanks, looks great!
I guess something might have to be done regarding the result shape, lowering will need to be able to query it from the result, but let's discuss how to best do this when that is needed.

jeanPerier accepted this revision.Feb 9 2023, 11:10 AM
This revision is now accepted and ready to land.Feb 9 2023, 11:10 AM
tblah added a comment.Feb 13 2023, 2:40 AM

@jeanPerier there's a problem building this with -DBUILD_SHARED_LIBS=ON. Putting the new helper functions in Builder/HLFIRTools.cpp requires a dependency from libHLFIRDialect.so to libFIRBuilder.so. But libFIRBuilder.so also depends upon libHLFIRDialect.so, making it circular.

Would it be okay to move the helper functions back to HLFIRDialect.cpp?

@jeanPerier there's a problem building this with -DBUILD_SHARED_LIBS=ON. Putting the new helper functions in Builder/HLFIRTools.cpp requires a dependency from libHLFIRDialect.so to libFIRBuilder.so. But libFIRBuilder.so also depends upon libHLFIRDialect.so, making it circular.

Would it be okay to move the helper functions back to HLFIRDialect.cpp?

Yes absolutely, sorry I did not catch that. All type related helper that are used in HLFIROps have to be defined in HLFIRDialect (that is why getFortranElementType is already there).

This revision was automatically updated to reflect the committed changes.