Great, thanks

In D131628#3901837, @nlopes wrote:

In D131628#3900066, @mysterymath wrote:

@nlopes, can we create a separate issue for finding the right semantics, and move forward by making this change describe as accurately as possible the current state of affairs?
Given that the original patch has been in LLVM for a long time, spanning many versions, it seems like we'd want to rectify the undocumentedness of the attribute ASAP, then work towards changing its semantics afterwards.

I think the effort of doing it right once is lower than doing it twice, but do as you see fit. As long as things get fixed..
Google doesn't have a good track record in fixing "temporary" hacks. The first time I approved one of such hacks was more than 10 years ago and the code is still there and google refuses to remove it. The other one had to be one of my students to fix it. Hence I'm a bit skeptical about fixing things "afterwards".

Please add more tests

Could you also mention this new otimization in release news?

I assume with this patch landed, many such cases may change code behavior. So we will need to update msan to have a tool to detect cases like this anyway.

Just FYI,

In D134410#3816918, @vitalybuka wrote:

I tried to hook this patch into MemorySanitizer and it reduces instrumented code by ~30% !

@nikic any comments from you?

Lets try it. If there are any problems, there is flag to disable this pass or revert this patch...

Are you gonna land this patch? Or any blockers?

Oh, right, sorry.

In rG09cdddea0c4d284c2c22f5dfade40a60850c5ea7#1126083, @marcauberer wrote:

@nlopes Oh, thanks for reporting. Seems there is a copy/paste mistake in the code. Fixed it here: https://reviews.llvm.org/D134135

Missing IR tests. No issues in codegen?

In D125142#3782244, @nickdesaulniers wrote:

@rsmith can we get some guidance here? Has your opinion changed in the time since GCC has been shipping this?

Current status? Review stalled?

Current status?

This commit caused following regression

Yeah, this looks as a great start. Better performance & compile times.

In D111968#3077358, @RKSimon wrote:

I'm still looking at refining the x86 costs

Isn;t this pattern handled by LoopFlatten? cc @SjoerdMeijer

existing cost model can drastically under-estimate the correct cost for the shuffle …. and some targets may require additions to the cost-model to correctly account for them.

Why? Reason?

Btw,

Was there any follow up? It seems that this hint (attribute) is currently not used by LLVM passes.

This should map to gcc's leaf attribute, right?

In D130894#3715124, @mstorsjo wrote:

This broke building with GCC (also noted by buildbot mails):

../tools/clang/lib/Sema/SemaDeclCXX.cpp: In member function ‘void clang::Sema::DiagnoseStaticAssertDetails(const clang::Expr*)’:
../tools/clang/lib/Sema/SemaDeclCXX.cpp:16666:19: error: declaration of ‘const clang::Expr* clang::Sema::DiagnoseStaticAssertDetails(const clang::Expr*)::<unnamed struct>::Expr’ changes meaning of ‘Expr’ [-fpermissive]
16666 |       const Expr *Expr;
      |                   ^~~~
In file included from ../tools/clang/include/clang/AST/DeclCXX.h:22,
                 from ../tools/clang/include/clang/AST/ASTLambda.h:18,
                 from ../tools/clang/lib/Sema/SemaDeclCXX.cpp:15:
../tools/clang/include/clang/AST/Expr.h:109:7: note: ‘Expr’ declared here as ‘class clang::Expr’
  109 | class Expr : public ValueStmt {
      |       ^~~~

Any progress?

Any tests with macros?

looks great

personally I dont see huge value to print “ left-hand side of operator '==' evaluates to …”

In D130894#3696534, @tbaeder wrote:

FWIW, complex numbers are already covered it seems:

test.cpp:35:1: error: static assertion failed due to requirement '__real c == __imag c'
static_assert(__real c == __imag c);
^             ~~~~~~~~~~~~~~~~~~~~
test.cpp:35:24: note: expression evaluates to 5 == 6
static_assert(__real c == __imag c);
              ~~~~~~~~~^~~~~~~~~~~
1 error generated.

In D125142#3505767, @jfb wrote:

I think the most relevant post from @rsmith is: https://discourse.llvm.org/t/making-ftrivial-auto-var-init-zero-a-first-class-option/55143/40

He has a prototype: https://reviews.llvm.org/D79249
I assume he would like someone to pursue it further, it was a good faith attempt at not just demanding. I'd played with it and it needed a few fixes, but overall it was pretty complete. Does someone want to give it a go?

This is not very friendly way how to remove things from LLVM. Code hygiene is a good thing but it should be reasonable and should not do more harm than good.

AArch64's sve tests

LG as well

In D130374#3675634, @vdsered wrote:

JFYI @spatel As I see this is quite similar to issue 53482. I remember some efforts to move out marking tail calls to a separate pass so this should be cheaper to do marking once again (not sure to what extent). It was not landed earlier, but it was ready to land as I understand :)

Do you plan to introduce new flag like gcc’s -fallow-store-data-races? They enable it only with -Ofast

In D127726#3659169, @eklepilkina wrote:

Maybe you could take a look at one big regression in MicroBenchmarks/LCALS/SubsetALambdaLoops/lcalsALambda.test:BM_PRESSURE_CALC_LAMBDA/5001

I had a look, I have already mentioned above that many of Microbenchmarks are very unstable. I ran several times for bothe versions and all results are in the same range for both cases. I compared binaries generated and they are the same for the binary that contains this microbenchmark.

So I also need to mention that some improvements also can be the result of this unstability of benchmarks.

Ah, right. Thanks for info.

Numbers look great.

Check https://reviews.llvm.org/D65457

Maybe reland with better patch title as this patch affects more libcalls than just two ones?

Does anybody see any major regressions?

We also see some use scenarios of TTI in InstCombine

Missing context
Missing test
x264_r regression is big one..
Many tests fail in precommit CI - please update them

-fsanitize=array-bounds workaround for size-1 array as the last member of a structure

In D128407#3604905, @troelsfr wrote:

Thanks for your comment.

You are completely right. I got misled by the output
SUMMARY: UndefinedbehaviorSanitizer, but after googling the message I
realised that this was a check added to integer in 2020 as you correctly
pointed out.

I am happy for this suggestion to be closed.

We don't want to limit the total number of characters being checked, but rather the number of of non-contiguous ranges.

Ping

In D127460#3601152, @GuillaumeGomez wrote:

This is my first LLVM contribution so I don't think I do? If I do have a commit access anyway, do you have a link to the documentation where it explains what I'm supposed to do by any chance?

Patch description needs to be added, especially answer the question why we should rename it, why we want it..

@dtemirbulatov thanks, looks neutral.

Update memchr.ll

strchr is transformed to memchr, if possible. See test strchr_to_memchr_n_equals_len.

In D128011#3591797, @nikic wrote:

This looks quite nice for the examples -- but can't this create a very large chain of comparisons in some cases? Like if you check every second character you'd end up with 128 comparisons? (Or maybe you need every third, I can imagine that we manage to fold every second character to am mask).

Added PhaseOrdering test to ensure instcombine + simplifycfg produces optimal switch code.