Skip to content

Conversation

@vzakhari
Copy link
Contributor

There is already a LIT test for hlfir.sum inlining that uses
the engineering option. I would like to keep the option
for short period of time to be able to revert
in case of performance regressions that I was not able to see.

There is already a LIT test for hlfir.sum inlining that uses
the engineering option. I would like to keep the option
for short period of time to be able to revert
in case of performance regressions that I was not able to see.
@vzakhari vzakhari requested review from jeanPerier and tblah December 13, 2024 23:51
@llvmbot llvmbot added flang Flang issues not falling into any other category flang:fir-hlfir labels Dec 13, 2024
@llvmbot
Copy link
Member

llvmbot commented Dec 13, 2024

@llvm/pr-subscribers-flang-fir-hlfir

Author: Slava Zakharin (vzakhari)

Changes

There is already a LIT test for hlfir.sum inlining that uses
the engineering option. I would like to keep the option
for short period of time to be able to revert
in case of performance regressions that I was not able to see.


Full diff: https://github.com/llvm/llvm-project/pull/119937.diff

1 Files Affected:

  • (modified) flang/lib/Optimizer/HLFIR/Transforms/SimplifyHLFIRIntrinsics.cpp (+2-1)
diff --git a/flang/lib/Optimizer/HLFIR/Transforms/SimplifyHLFIRIntrinsics.cpp b/flang/lib/Optimizer/HLFIR/Transforms/SimplifyHLFIRIntrinsics.cpp
index 3e9d956b6e56dd..a90c5a9ef5a4e5 100644
--- a/flang/lib/Optimizer/HLFIR/Transforms/SimplifyHLFIRIntrinsics.cpp
+++ b/flang/lib/Optimizer/HLFIR/Transforms/SimplifyHLFIRIntrinsics.cpp
@@ -33,7 +33,8 @@ namespace hlfir {
 static llvm::cl::opt<bool>
     simplifySum("flang-simplify-hlfir-sum",
                 llvm::cl::desc("Expand hlfir.sum into an inline sequence"),
-                llvm::cl::init(false));
+                llvm::cl::init(true));
+
 namespace {
 
 class TransposeAsElementalConversion

@vzakhari
Copy link
Contributor Author

Hi @tblah, could you please verify that there is no exchange2 performance regression any more? I measured it on my side, and performance was the same with and without hlfir.sum inlining.

FWIW, I do see some difference in LLVM inlining:

$ nm noinline.exe | grep digits_2
000000000003b7a0 t _QMbrute_forcePdigits_2.specialized.4
$ nm inline.exe | grep digits_2
000000000003aba0 t _QMbrute_forcePdigits_2.specialized.3

Copy link
Contributor

@tblah tblah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much for taking the time to fix our performance regression. I don't see any regression after enabling this pass now.

@vzakhari
Copy link
Contributor Author

Thank you for checking, Tom!

@vzakhari vzakhari merged commit f239922 into llvm:main Dec 16, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

flang:fir-hlfir flang Flang issues not falling into any other category

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants