Page MenuHomePhabricator

[FLANG]Add maxval simplification support
ClosedPublic

Authored by Leporacanthicus on Aug 19 2022, 7:44 AM.

Details

Summary

Add simplifcation pass for MAXVAL intrinsic function

This refactors some of the code to allow variation on the
initialization value and operation performed within the loop,
reusing the majority of code for both SUM and MAXVAL.

Adding tests for the test-cases that produce different output
than the SUM function.

Diff Detail

Event Timeline

Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptAug 19 2022, 7:44 AM
Herald added a subscriber: mehdi_amini. · View Herald Transcript
Leporacanthicus requested review of this revision.Aug 19 2022, 7:44 AM
vzakhari added inline comments.Aug 19 2022, 9:43 AM
flang/lib/Optimizer/Transforms/SimplifyIntrinsics.cpp
43

This should not be needed. See comments in genFortranAMaxvalBody.

71

typo: returngin

84

Please add function header comment.

120
121

nit: maybe renamed reduction to accumulator or reductionValue.

160

I would rather pass loc to here from the caller, so that we do not have 3 different places where we define location for the function body elements. I understand that it will probably be unknown always, but it should still be better to define the location in one place.

172

I would rather just use return here and at 168. Then you can get rid of else if as well.

189

Please use getLargest(/*Negative=/true) instead.

190

Please use getSignedMinValue instead.

Leporacanthicus marked 4 inline comments as done.

Review updates:

  • Use different method to find a smallest value to start for MAXVAL
  • Tidy up some code in general.
  • Add descriptive comment to big function.
  • Pass loc around in a few places.
Leporacanthicus marked 5 inline comments as done.Aug 22 2022, 12:10 PM
Leporacanthicus added inline comments.
flang/lib/Optimizer/Transforms/SimplifyIntrinsics.cpp
189

Thanks. I tried looking for these type of functions, and for whatever reason, I couldn't find in either LLVM APFloat docs or source files.

I also asked on Slack, but nobody answered. I will update the thread with this

flang/test/Transforms/simplifyintrinsics.fir
757

Wrong number of ----, I've fixed it!

This revision is now accepted and ready to land.Aug 22 2022, 2:34 PM

Two comments regarding sharing code.

flang/lib/Optimizer/Transforms/SimplifyIntrinsics.cpp
551

Is this whole if statement shareable with Sum?

555–561

I guess this type deduction can be a common function, sharing code with Sum.

Leporacanthicus marked an inline comment as done.Aug 24 2022, 5:26 AM
Leporacanthicus added inline comments.
flang/lib/Optimizer/Transforms/SimplifyIntrinsics.cpp
551

Yes, it is. I have another patch coming that moves these things around, and reduces the code duplication.

555–561

I think it should be rewritten to use a different mechanism, but in the meantime, the patch I talked about above will remove the duplicated code.

This revision was automatically updated to reflect the committed changes.