This is an archive of the discontinued LLVM Phabricator instance.

[FLANG][NFCI]De-duplicate code in SimplifyIntrinsics
ClosedPublic

Authored by Leporacanthicus on Aug 24 2022, 12:13 PM.

Details

Summary

This removes a bunch of duplicated code, by adding an intermediate
function simplifyReduction that takes a std::function argument
for the actual replacement of the code.

No functional change intended.

Diff Detail

Event Timeline

Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptAug 24 2022, 12:13 PM
Herald added a subscriber: mehdi_amini. · View Herald Transcript
Leporacanthicus requested review of this revision.Aug 24 2022, 12:13 PM

Updated comment for helper function

vzakhari added inline comments.Aug 26 2022, 3:16 PM
flang/lib/Optimizer/Transforms/SimplifyIntrinsics.cpp
53

I was recently told that we do not usually use std::function. The advice was to replace it with function_ref: https://reviews.llvm.org/D129810#inline-1277689

75

typo: hte

76

typo? simplifyd -> simplified or simplify?

Leporacanthicus marked 2 inline comments as done.

Updates per review comments:

  • Fix typos
  • Use llvm::function_ref instead of std::function

Thanks for function_ref replacements!

flang/lib/Optimizer/Transforms/SimplifyIntrinsics.cpp
76
flang/lib/Optimizer/Transforms/SimplifyIntrinsics.cpp
53

Ok, there's only 5 in this section of code, I will fix those.

There's 115 std::function and 10 llvm::function_ref in flang - not making excuses here, but I think a separate cleanup patch may be required.

I will see if I can fix that separately after this has gone in as a "good citizen" patch.

vzakhari added inline comments.Aug 31 2022, 9:58 AM
flang/lib/Optimizer/Transforms/SimplifyIntrinsics.cpp
53

Sure, the other places can be cleaned up separately.

Added const to KindMapping argument.

Leporacanthicus marked an inline comment as done.Aug 31 2022, 12:44 PM
Leporacanthicus added inline comments.
flang/lib/Optimizer/Transforms/SimplifyIntrinsics.cpp
76

I'm sure I fixed this before now. Anyway, thanks for spotting!

vzakhari accepted this revision.Sep 1 2022, 8:14 AM

Thanks for the changes! LGTM

This revision is now accepted and ready to land.Sep 1 2022, 8:14 AM
This revision was automatically updated to reflect the committed changes.