In D88307#2342917, @eastig wrote:

In D88307#2340132, @alanphipps wrote:

@eastig I tried this experimental patch in our system, and it definitely brings highly significant improvement to coremark. When do you expect to post a version for reviewing -- what will the differences be?

Hi Alan,
Thank you for the feedback. I need to get an approval from my current employer to continue working on the patch. I'll keep you posted.

In D88307#2340132, @alanphipps wrote:

@eastig I tried this experimental patch in our system, and it definitely brings highly significant improvement to coremark. When do you expect to post a version for reviewing -- what will the differences be?

Do you mean to improve the existing LLVM Jump Threading pass?

Yes, either improve current one or write new more powerful JT pass.

In D88307#2311350, @xbolva00 wrote:

Did you consider to implement more Agressive Jump Threading instead of a new pass for specific use (FSMs)? Like following solutions:

• gcc/libfirm jt http://beza1e1.tuxen.de/articles/jump_threading.html
• codasip jump threading https://codasip.com/wp-content/uploads/2019/08/Codasip_Jump_Threading.pdf

In D88307#2310500, @amehsan wrote:

@alexey.zhikhar and I had a look at your patch. Some findings that might be interesting:

1- The IR from your patch is shorter than ours. We haven't checked carefully where the difference is, but there is a gap of about 10% (of the size of core_state_transition)
2- Our code from our current implementation is about 2%-3% faster on our platform.
3- While the main idea is exactly the same our implementation looks at all the control flow paths in the loop first and perform the analysis. Then we have a code gen process. Your approach is closer to the existing jump threading in that you make one change at a time.
4- As I said, we are making some significant changes in our implementation to improve the code quality (among other things), etc. So our new code that we will be able to share is not ready yet.

Thanks for sharing your code.

This patch is not supposed to be used as is. It can be used for experiments and as a proof of concept. A version for reviewing will be submitted soon.

In D31413#2277133, @ldionne wrote:

In D31413#2277118, @eastig wrote:

In D31413#2277070, @ldionne wrote:

Added a test. I can't reproduce the issue, so I don't know whether the test is useful or not. Please help with that!

There are tests in https://reviews.llvm.org/D12689 .

Oh, nice, I missed that. I'll borrow those if you don't mind. Can you close https://reviews.llvm.org/D12689 and leave a trace to this review? I'm trying to de-tangle things here.

In D31413#2277070, @ldionne wrote:

Added a test. I can't reproduce the issue, so I don't know whether the test is useful or not. Please help with that!

In D56720#1398464, @asbirlea wrote:

FWIW, the cause of the slowdown is the usage of MemorySSA in EarlyCSE.
Here's an alternative solution: D58248.
On my machine compile time goes from ~41s to ~11s (with vixl ToT today).

Hi George,

+1, LGTM
I'd like someone from the X86 world to approve this as well.

Hi Jonas,

Hi Jonas,

In D46723#1096542, @eastig wrote:

In D46723#1096058, @jfb wrote:

In D46723#1096025, @eastig wrote:

We might need to play with _GLIBCXX_USE_CXX11_ABI=0 and the -Wabi-tag option to mitigate libstdc++ ABI issues.

Agreed. Do you have a Linux machine to try it out? I don't have a VM set up with an older distro at the moment. It would be useful to see which libstdc++ features are used by LLVM when compiled with GCC 5.1 (as well as with LLVM 3.4 when using libstdc++).

I have a machine with Ubuntu 14.04. I'll run experiments.

In D46723#1096058, @jfb wrote:

In D46723#1096025, @eastig wrote:

We might need to play with _GLIBCXX_USE_CXX11_ABI=0 and the -Wabi-tag option to mitigate libstdc++ ABI issues.

Agreed. Do you have a Linux machine to try it out? I don't have a VM set up with an older distro at the moment. It would be useful to see which libstdc++ features are used by LLVM when compiled with GCC 5.1 (as well as with LLVM 3.4 when using libstdc++).

We might need to play with _GLIBCXX_USE_CXX11_ABI=0 and the -Wabi-tag option to mitigate libstdc++ ABI issues.

Sanjay,
Maybe it's worth to send the API change to llvm-dev as RFC?

Just validated that it has fixed the issue.
Thank you, Peter.

Hi Peter,

Hi Sanjay,

Hi Sanjay,

Ping

Ping

Hi Eli,

Hi Peter,

In D41578#984613, @skatkov wrote:

In D41578#984351, @eastig wrote:

In D41578#981582, @skatkov wrote:

Evgeny, I posted an alternate solution https://reviews.llvm.org/D42290. Could you please check whether it fixes your regression and do not introduce others?

Hi Sergey,
I checked it fixes regressions. Could the test I posted here be added as a regression test to D42290?

Did you check whether it introduces new regressions?
I will add. Anyway I will wait for review from Sanjoy...

LGTM

In D41578#981582, @skatkov wrote:

Evgeny, I posted an alternate solution https://reviews.llvm.org/D42290. Could you please check whether it fixes your regression and do not introduce others?

In D42263#982004, @jgalenson wrote:

Thanks, Evgeny! I'll fix the comment and commit it now and then continue to work on cleaning up this loop.

Hi Joel,

In D38378#980891, @eastig wrote:

In D38378#980879, @jgalenson wrote:

What command-line arguments are you using?

-O3 -DNDEBUG -mcpu=cortex-a9 -mthumb -fomit-frame-pointer

In D38378#980879, @jgalenson wrote:

What command-line arguments are you using?

In D38378#980789, @jgalenson wrote:

The example I found is in equal_data, which I assume is being inlined in make_list.

I have a (one-line) fix for that, and I'm trying to run the test suite to see what else it improves. However, I'm not seeing any changes in fourinarow, and I don't immediately see any obvious bad code in it. Do you happen to know where the regression in it was?

Thank you, Joel. The function to look at is make_list.

In SingleSource/Benchmarks/McGill/chomp the patch causes generation of subs+cmp_with_0 instead of only a subs.

The patch caused regressions in LNT benchmarks on Cortex-A9:

In D41578#979947, @skatkov wrote:

For "%mul86.i = mul nuw nsw i32 %p280, %p281" isSCEVExprNeverPoison return false which causes the SCEV '(%p280 * %p281)' to have SCEV::FlagAnyWrap instead of a combination of SCEV::FlagNUW and Flags, SCEV::FlagNSW.

Am I correct SCEV::FlagAnyWrap means no poisoned values? If so, this looks strange. Shouldn't it be:

As I understand this flag means have no idea, in other words may overflow.

Hi Sergey and Sanjoy,

A reproducer:

In D41578#975919, @skatkov wrote:

Hi Evgeny, please let me know if you need some more deep explanation about workaround Sanjoy and I mentioned in terms of stripping nuw/nsw for instruction in case you are in LSR.

This is for the case: the regressions are urgent for you and you need some quick workaround while you are investigating why SCEV lost the poison flags.

I have found that the patch also caused 7.31% regression in SPEC2k6 401.bzip2 on Cortex-A57 (AArch64).

Sergey,
Thank you for the initial analysis. I'll try to debug SCEV.

Hi Sergey,

The patch looks OK to me if using std::to_string is allowed now.

In D41324#958398, @ashahid wrote:

In D41324#958204, @eastig wrote:

In D41324#957637, @ashahid wrote:

Regression and LNT test was successful. Bootstrap test is underway.

Hi Shahid,

Do you mean the LNT test code is back to the code before the patch D36130?

Thanks,
Evgeny

Sorry, I could not get your question.
Recently I started running these lit test, LNT test and bootstrap test to make sure build bot does not break upon commit.

Regards,
Shahid

LGTM

In D41324#957637, @ashahid wrote:

Regression and LNT test was successful. Bootstrap test is underway.

Hi Shahid,

In D36130#955158, @eastig wrote:

Hi Shahid,

These changes caused 27.7% and 30.2% regressions on an AArch64 Juno board (http://lnt.llvm.org/db_default/v4/nts/83681):

MultiSource/Benchmarks/mediabench/gsm/toast/toast: 30.20%
MultiSource/Benchmarks/MiBench/telecomm-gsm/telecomm-gsm: 27.73%

We have the same benchmarks regressed on our AArch64 boards (Cortex-A53, Cortex-A57).

-Evgeny Astigeevich
The ARM Compiler Optimisation team

Hi Shahid,

Hi Nirav,

In D40701#943786, @niravd wrote:

I've disable store merges of FP constants with truncation as it's not clear what is the right behavior at the moment.

Evgeny, can you please test that this works for you?

There are stability and correctness issues on AArch64. The similar issues might exist on other ARM targets. Could you please disable MergeConsecutiveStores for all ARM targets including AArch64?

These changes caused failures of AArch64 NEON Emperor tests.

These changes caused Clang to crash when it compiled spec2006 403.gcc for AArch64. I am working on a reproducer.

LGTM

A reproducer:

Hi Nirav,

@evgeny777: I'll try to find some time to look at 401.bzip2.

In D39415#919103, @evgeny777 wrote:

401.bzip2: 5.54% code size increase

That's really strange, any idea how this could happen?

I've got Spec2006 results for Cortex-A57:

Cortex-A57 results from another private benchmark (50 sub-benchmarks):

In D39415#915116, @rengolin wrote:

Interesting. I'm guessing all TSVC reductions are due to the same function.

It's also interesting that TSVC was the code that changed the most, and had no visible performance benefits.

In D39415#914995, @rengolin wrote:

Not very convincing numbers. I guess efficient pipelining can make most of the difference wash away.

I've got first results of benchmark runs: the LNT test suite + a private benchmark 01. I used the latest patch.
The configuration is a Juno board Cortex-A57/A53, v8-a, AArch32, Thumb2.
Options: -O3 -mcpu=cortex-a57 -mthumb -fomit-frame-pointer
The runs passed without errors.

I'll do AArch32/AArch64 benchmarks runs on our Juno boards. It would be worth to run CTMark to check compilation time.

Hi Max,

Hi Max,

I attached

The benchmark source file: http://www.llvm.org/viewvc/llvm-project/test-suite/trunk/SingleSource/Benchmarks/Shootout/sieve.c?view=markup

Hi,
This patch caused SingleSource/Benchmarks/Shootout/shootout-sieve regression on Arm public bots:

The patch has caused a few regressions on Arm too. They are within 5%. We investigated some of them. For example, the unrolling is not applied. However the unrolling could figure out that some non-free GEPs are removed during optimisation. Maybe other passes have the similar problem. We currently need more precise cost model for inlining at Oz. However we started looking at the recently introduced TTI code size cost if we can use it.
At which optimisation level is the inlining affected? Could we fix the issues by updating the heuristics?

Thank you, Justin.

Hi Justin,

Thank you, Jun, for fixing this.
There is PR33642 with a test case when TCC_FREE is returned for non-cost free GEPs.

Thank you, Eli.

Ping

Thank you, Renato, for clarifying that.

In D37377#858689, @rengolin wrote:

As I said in D37374, please refrain from approving your own patches within minutes of posting. The community must be part of the process, and if people are not reviewing your patches as fast as you want, then there's either something wrong with the community (and we need to fix), or with your expectations.

If the problem is in the community, then the documented approach to make people care about your work is to care about theirs first. The more you review other people's patches, the more likely they are to review yours. If that doesn't work (after months), then we should discuss the problems in the dev list.

LGTM.
BTW the issue was found during porting micropython for Cortex-M0 from GCC to Clang.

As this patch can affect ARM targets I am doing some benchmarking.
I've got the LNT benchmarks results for AArch64 (Cortex-A57). There is no difference in performance. I'll have got more results soon.
It's interesting to see what benchmarks has been used to measure the improvements.

LGTM
Thank you, Haicheng.

Thank you, Eli.