There shouldn't be an empty string ("") in the asm output -- that should be a reference to the "l_yes" label, not the empty string. That seems very weird...

Ping.

Ping.

Thanks!

If you change to condition on GLIBC instead of linux, I think this is fine to commit (although it still seems unfortunate to me that we're carrying an abandoned codebase which depends on weird broken stuff like this...)

Looks reasonable to me.

Can you please write a test case for this? There's some existing sparcv9 spill tests in llvm/test/CodeGen/SPARC/64spill.ll.

This commit message is rather bare-bones -- I hope that the ultimate commit message will be updated to contain more than just a reference to the RFC thread.

This patch is not correct -- the crash is not with varargs functions specifically, llvm also crashes for a function declared to explicitly take a float/double. The Win64 calling convention code assigns values to xmm registers, but it should not when sse is disabled.

After looking at this a bit more, I realized that this is still insufficient. There's *many* more target-specific constraints than just x86 "i" which sign extend constant integer arguments -- and we'll need to apply this change to every one of them, in all targets.

This looks like the same bug is present in the generic code in llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp as well.

I would question whether it's actually worth it to attempt to keep software working that seems to have been abandoned for a decade as part of the llvm test suite? Should this code just be removed, instead of patched?

Sounds like this file is missing #include <math.h> (it probably only #include <cmath>). If it wants to use ::log10, it should #include <math.h>. If it wants to use <cmath>, it should use std::log10.

It'd be nice if we knew why adding new nodes to the worklist seemed to cause an issue, but the reduced form as here seems okay as far as it goes.

Looks fine again.

Not sure if the PPC issue was a different one, but the commit that reverted this was:

In D44373#1036189, @steven_wu wrote:

Thanks for fixing this. LGTM.

Could you add a comment to the CL description noting that a bunch of the calls to AddToWorklist in DAGCombiner.cpp that follow a DAG.getNode() are now redundant, and should be cleaned up in the future.

Sorry, forgot to re-ping this in a timely manner. :)

Looks good.

It looks like this was generated because a pull request against the upstream repo was created containing this commit.

Ah -- I now understand your concern, and managing user expectations appropriately does seem like a potential concern.

Is there any reason this needs to be a template -- can't these just be changed to function overloads, instead of template specializations?

The errors disabled by this feature are default-error warnings -- you can already get the same effect by using -Wno-<lots-of-things>. Why is it bad to additionally allow -fpermissive to disable them? (If we have any diagnostics which are currently default-on-warnings which should not _ever_ be disable-able, then maybe we should just fix those?)

Hm -- in GCC, -fpermissive has nothing at all to do with -pedantic/-pedantic-errors, but I suppose it should be fine to do this way.

The intricate initialization-order workarounds apparently don't work in all build modes, so I've updated this code to have constexpr functions and initializations in r355278.

Probably worth adding a note to the release notes, something like "The optimizer will now convert calls to memcmp into a calls to bcmp in some circumstances. Users who are building freestanding code (not depending on the platform's libc) without specifying -ffreestanding may need to either pass -fno-builtin-bcmp, or provide a bcmp function."

Minor tweaks per comments.

In D58548#1407164, @dexonsmith wrote:

I like this idea, and I don’t think the textual IR central is too important. A few things:

• Changes to the IR should always be discussed on llvm-dev. Did this already happen?

Add some wording to LangRef and clang-format.

In D57601#1397317, @reames wrote:

Question for Reviewers: Going through uses of isVolatile in backend code, I see suspicious bits in Lania, ARM, and Hexagon. What's the best way to handle this? I can easily apply the workaround I mention for out of tree backends, or I could make them equally conservative for isAtomic. The other in tree backends appear to be fine, though I'd welcome review from folks knowledgeable of non-x86 backends.

I still don't like it...It's different, unusual, and IMO surprising to have such a wildcard ignore.

In D58260#1399596, @efriedma wrote:

I'm concerned that "nocapture" is not sufficient to describe the necessary property here.

I think this warning (-Wbuiltin-requires-header) doesn't really make sense as its own warning.

OK, I'm happy with all of that.

Looks reasonable to me.

It'd be great to see this somewhat more widely publicized, outside of just the clang community. If libc implementors are aware of the gains and are willing to provide an actually-faster bcmp implementation, it'd be a lot better, than having this optimization that doesn't really optimize anything without users providing their own bcmp implementation.