[flang][hlfir] Add hlfir.maxval intrinsic #65705
Conversation
|
FYI: My colleague or I will implement |
| @@ -404,6 +404,32 @@ def hlfir_CountOp : hlfir_Op<"count", [AttrSizedOperandSegments, DeclareOpInterf | |||
| let hasVerifier = 1; | |||
| } | |||
|
|
|||
| def hlfir_MaxvalOp : hlfir_Op<"maxval", [AttrSizedOperandSegments, | |||
| DeclareOpInterfaceMethods<ArithFastMathInterface>]> { | |||
There was a problem hiding this comment.
Please could you add memory effects like I did in https://reviews.llvm.org/D158662. This is new since the previous intrinsics were added.
You will need to DeclareOpInterfaceMethods<MemoryEffectsOpInterface>, add an implementation of MaxValOp::getEffects which calls getIntrinsicEffects and add a test to memory-effects.fir
There was a problem hiding this comment.
Thank you for your comment and information.
I fixed it.
flang/lib/Lower/HlfirIntrinsics.cpp
Outdated
| auto argCharType = mlir::dyn_cast<fir::CharacterType>( | ||
| hlfir::getFortranElementType(argArray.getType())); | ||
| if (argCharType && argCharType.hasConstantLen()) | ||
| resCharType = fir::CharacterType::get( | ||
| builder.getContext(), resCharType.getFKind(), argCharType.getLen()); |
There was a problem hiding this comment.
Could you add a TODO comment to improve the front-end analysis here?
I see that it is not able to fold len(maxval(array(:, :)(:))) when array length is a compile time constant, and I think it should even though this is not technically a constant expression as per 10.1.2 since the len argument is not a variable.
I do not have an immediate idea of why this is not resolved by semantics currently. Some folding step may be missing.
It is likely fine to assume that all transformational operating on a character arrays return an object with the same length, but I would rather leave that up to semantics for the future sake.
There was a problem hiding this comment.
Thank you for your comment. It's very helpful for me.
After applying your patch #65771 to my branch, it seemed that my implementation for length information came to be unnecessary.
Should I remove this? If so, is the TODO comment still needed?
There was a problem hiding this comment.
Thanks for the review on this patch, I just merged it, so you should just be able to remove the part dealing with the constant length.
There was a problem hiding this comment.
Thank you for your reply.
I fixed it.
yus3710-fj
force-pushed
the
feature/hlfir.maxval
branch
from
bd39bc0 to
653e949
Compare
|
Now the check fails because this branch hasn't been rebased yet in order to keep changes easy to track. |
yus3710-fj
force-pushed
the
feature/hlfir.maxval
branch
from
52ce3f9 to
39979cc
Compare
Adds a new HLFIR operation for the MAXVAL intrinsic according to the design set out in flang/docs/HighLevelFIR.md.
Adds a new HLFIR operation for the MAXVAL intrinsic according to the design set out in flang/docs/HighLevelFIR.md.
Adds a new HLFIR operation for the MAXVAL intrinsic according to the design set out in flang/docs/HighLevelFIR.md.