Doing a websearch for "xtensa isa", I do see that documentation does seem to exist: https://github.com/eerimoq/hardware-reference/blob/master/esp32/xtensa%20Instruction%20Set%20Architecture%20(ISA)%20Reference%20Manual.pdf.

+1 for doing this. I started looking at fixing this when I modified the printer to print proper labels for numbered basic-blocks (instead of comments), but I didn't do so because of the amount of test churn was off-putting.

With this new pass, I think we should be able to remove the code handling the objectsize and is.constant instrinsics in CodegenPrepare, FastISel, SelectionDAGBuilder.cpp, and IRTranslator. Which sure is a nice cleanup.

I think the pass needs to handle the removal of any remaining llvm.objectsize, as well, so that llvm.is.constant(llvm.objectsize(...)) continues to return true -- even if the object size cannot be determined.

.

I don't really want to promise any particular version, because nobody is really checking that particular versions of git work -- the way our scripts continue work with old versions is when developers use them and report a problem. The only real requirement here is "whatever the oldest version someone's actually used recently".

Given the reference to the bug, seems fine to me to commit an xfail here since nobody is actively working to fix this right now.

I'm not sure the history behind why these were added as default-on warnings....they don't really seem appropriate as default warnings to me, either.

I fear it is necessary: at least it matches documented behaviour of both the Sun/Oracle Studio compilers and gcc.

Is this really necessary? Users don't typically pass -std= to the driver for linking anyways (what do you even pass if you've compiled both C and C++ code?) so this seems a rather odd way to control behavior.

In D65123#1596983, @labath wrote:

In D65123#1596935, @teemperor wrote:

Thanks for restoring this, but I'm a bit confused how this has happened. My original diff didn't remove these folders, but it seems when I committed the diff via arcanist and git llvm it also removed these other folders. I assume git llvm is when translating the change to SVN not just removing a folder but also all folders in the same directory?

Interesting.. I wouldn't be totally surprised by that as git-llvm does some magic to svn check out only the files/directories that have been modified. Combining that with the different treatment of directories in svn and git (explicit modelling vs. implicit "it exists if it contains a file"), I can see how things might go south in some corner cases like this. But that's just speculation -- I'm generally trying to avoid git-llvm as it is too magical for my taste. Maybe @jyknight or @mehdi_amini can shed more light on this?

The code you reference in GCC is only for trampolines, while this function is used to implement builtin_clear_cache. AFAICT GCC is just wrong to implement this as a no-op.

LGTM.

I agree, it should have one, especially once we stop supporting separate checkouts, and hopefully can remove redundant copies in other places.

Actually linker scripts do support metacharacter escaping (although, it seems lld does not support this correctly, yet).

Comment updates SGTM.

I like it. Some minor suggestions for a few extra comments, but other than that LGTM.

I don't think this was correct (where by "correct", there, I mean "what GCC does", as this patch is intended to match GCC behavior).

I don't really have much to say about this, and the patch is probably fine, but I do note that most of the other accessors on this class also return mutable objects.

One of the other suggestions was to pass a _type_ as a parameter to byval. IMO that would be the nicest idea (but I don't know if it's infeasibly difficult?)

Sorry for dropping this, a couple more comments, then I think it's good.

In D62268#1512672, @ldionne wrote:

This LGTM.

When I did the refactor, all the code was only using -isysroot (and never accessing --sysroot), so I thought only -isysroot was relevant on Darwin. Seems like I was wrong.

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.