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..

-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.

What do you think about just using the clang AST and checking if there is a valid definition for __memcmpeq + isGNUEnvironment?

Seems fine as well.

Thanks

Maybe new flag like -fglibc-version=ver ?

Ideally LLVM should just "enable" memcmpeq if glibc version is > XYZ. Not sure if there is a realiable way to do it. @efriedma ?

I guess we can go with the flag being used by gcc: -fextra-libc-function=__memcmpeq.

In D125723#3585726, @aaron.ballman wrote:

LGTM! But given the number of people who've expressed opinions on this, please wait a day or two before landing in case other reviewers have additional feedback.

Clang side should match gcc’s https://gcc.gnu.org/pipermail/gcc-patches/2022-June/596335.html

Also note that there is a '#pragma GCC optimize' pragma. After this patch, it should be hard to implement it.

See what you mean and why it might be necessary.

@courbet Any followup as we have got "'__memcmpeq" ?

I would like to hear quite strong motivating words.

That's not stopped us from exposing attributes like minsize (and you proposed optsize as well) which get described in the same terms as -Os/-Oz in our documentation, so this ship has somewhat sailed.

In D125723#3547789, @rnk wrote:

I think it's fine to implement this, but I wanted to share some of the motivation for the current state of things. In our experience, the majority of uses of pragma optimize were to work around MSVC compiler bugs. So, instead of honoring them, we felt it was best to ignore them with a warning (-Wignored-pragma-optimize). Of course, that warning is typically disabled in build systems of large projects, so our current behavior can still surprise users. Implementing the feature is probably the most predictable and least surprising thing Clang can do.

Something we can't easily support for either GCC or MSVC attribute is enabling optimizations for some functions in an -O0 build. Maybe it's now possible to set up a full pass pipeline after semantic analysis is complete, but it's not straightforward. You should consider what to do with that corner case.

In D126984#3581708, @aaron.ballman wrote:

In D126984#3579842, @jdoerfert wrote:

In D126984#3571573, @aeubanks wrote:

IIRC in the past there was a strong preference to not have the pass manager support this sort of thing
if you want to support this, there should be an RFC for how the optimization part of this will work as it may require invasive changes to the LLVM pass manager

(if this is purely a clang frontend thing then ignore me)

We actually should *not* make this a clang frontend only thing. It is confusing and not helpful. That said, we have code to integrate this into the new PM already as we were planning on proposing something along these lines too. We didn't manage to get to it during last years GSoC but the code could be used as a basis still.

+1 for RFC
strong preference for proper integration of this into the new PM.

I'm not opposed to an RFC to extend this functionality, but it seems to me that we have incremental progress already with this patch and landing this patch unblocks the work @steplong was originally doing for the MSVC pragma. Do you have a concern if we move forward with this less-functional form so that work isn't held up on an RFC for the more fully functional form?

If you compile file with -Ofast and use optimise(-Os) for F - I would expect no fast math flags for function F but I am worried a bit that only “optsize” is added. Plesse verify and/or add such test.

Would that work for you (and you as well @aeubanks)?

The patch should not be blocked by other patches, or? or any blocker?

But where I think this feature could be very useful in following case from gcc test suite where there is some FP computation..
Imagine you compile you program with -ffast-math and then you have function:

Are you opposed to exposing #pragma optimize? (https://docs.microsoft.com/en-us/cpp/preprocessor/optimize?view=msvc-170) If yes, I think Stephen should run an RFC on Discourse to see if there's general agreement.

No, I like it, seems more useful and general than "pragma clang optimize on/off"

Are you opposed to the direction of condensing the optimization semantic attributes (the things in the AST) down into one? If yes, I'd like to understand why better.

No :)

Are you still opposed to exposing a neutered form of the GCC optimize attribute as a parsed attribute (the thing users write in their source)? If yes, that's fine by me, but then I'd still like to see most of this patch land, just without a way for the user to spell the attribute themselves. We can adjust the semantic attribute's enumeration to cover only the cases we want to support.

"The optimize attribute should be used for debugging purposes only. It is not suitable in production code."

In D126984#3574071, @xbolva00 wrote:

I agree with @aeubanks , this feature requires major changes to pass manager and I see no value to land this currently.

I see that somebody may prefer “opt for size”, but this is exposed via “minsize” attribute so I see no strong need for optimize(“-Os”)

[[gnu::optimize("O0")]] is okay but [[gnu::optimize("O3")]] is not gonna work without major changes. Not sure why we should deliver half-broken new attribute.

I agree with @aeubanks , this feature requires major changes to pass manager and I see no value to land this currently.

Is it possible to create simple testcase?

in AddAliasScopeMetadata: the default value of MaxLookup is not enough to identify the underlying object, and the alias info added by AddAliasScopeMetadata() is wrong, resulting in wrong schedule decisions in machine-scheduler.

Some unfortunate regressions

Cases like strlen (x ? s1 : s2) should be handled in more general fashion and rewrite fn (x ? s1 : s2) as x ? fn(s1) : fn(s2) if fn(s1) and (or?) fn(s2) can be optimized to just some (constant) value.

Maybe also worth to check gcc's issues with this transformation:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91734

Is this “safe” even with fast math for all predicates?

Botan AES-128 benchmark, maybe others..

LG

Please rebase.

also Polly.DeLICM::pr41656.ll fails in precommit CI.

This could be done by either exposing a simplifyLoopCFG helper function ...

In D125078#3503921, @cjdb wrote:

I support turning it on by default. I'm simply passing on the message.

In D125078#3501061, @cjdb wrote:

I'm in favour of this, but @rsmith warned me a little while ago that we mightn't be able to have stuff like this on by default because scripts probably depend on specific compiler output. Hyrum's Law strikes again :(

(I personally think we should have it on by default and give a way to opt out, rather than opt in.)

In D125223#3501111, @haberman wrote:

It is not safe to change this in Clang only. As mentioned in https://github.com/llvm/llvm-project/issues/54964#issuecomment-1101667557, these rules would need to be loosed at both the Clang and LLVM levels.

If we change Clang only, it will produce LLVM IR that will fail to verify: https://github.com/llvm/llvm-project/blob/09c2b7c35af8c4bad39f03e9f60df8bd07323028/llvm/test/Verifier/musttail-invalid.ll#L13-L18

In D124633#3501083, @msebor wrote:

This revision looks good to me. I suggest reviewing the rest of the folders for missing annotations and adding those in a follow up patch.