Thanks, I am seeing improvements with this new version. I'll try to go over the changes in more detail later, some initial superficial comments now.

In D127119#3564522, @ABataev wrote:

In D127119#3564461, @hvdijk wrote:

Comparing the changes to the tests in this diff to those in D127073, I am seeing a number of tests where we have more shufflevectors, and none where we have fewer. Are there improvements that are not as obvious to see?

Yes, there are. These extra shuffles caused by changes in performExtractsShuffleAction() and in IsIdenticalOrLessDefined lambda, these changes (they treat UndefMaskElem as possible poison) increase number of shuffles. Without them, there are less shuffles, these extra changes are required for correct handling of UndefMaskElem as posion.

Comparing the changes to the tests in this diff to those in D127073, I am seeing a number of tests where we have more shufflevectors, and none where we have fewer. Are there improvements that are not as obvious to see?

The updates to the tests suggest that this doesn't just fix the broken cases, it generates worse code where the previously generated code was already correct: I'm seeing insertelements into a vector immediately followed by a shufflevector that discards the inserted element, where previously those elements were not set. Is there some way that we can avoid that?

In D126895#3554064, @vporpo wrote:

I think that the issue boils down to whether: %zext = zext i8 poison to i32 is an i32 poison.

The value coming from an unreachable block from entry is uninitialized

In D122663#3471741, @erichkeane wrote:

Ping me EOW if @rsmith doesn't respond in the meantime.

Add a comment explaining why we do not use address size overrides here.

Clean up test slightly to load consecutive i32 values like the i64 version did

In D122663#3471866, @erichkeane wrote:

This actually wouldn't be necessary in this case: cxxfilt is already 'right', this is just for humans to be able to see "we changed this from mangling as <obviously wrong> to <looks right>".

In D122663#3471763, @erichkeane wrote:

I DID see that and am REASONABLY sure you do, but sometimes folks will ask for test coverage because they suspect the resulting behavior will demonstrate some sort of problem with the current patch. I DID run this through the demangler and found that the PRE15 case looks odd?

void test10<X>(X::Y::a, X::Y::b, float*, X::Y)
vs
void test10<X>(X::Y::a, X::Y::b, float*, float*)

Or is that exactly the bug being caught here? That is, is the float* substitution not being properly registered/emitted and being confused with X::Y?

In D122663#3471741, @erichkeane wrote:

Ping me EOW if @rsmith doesn't respond in the meantime. It is also not clear to me whether you were able to capture/fix the issue he had with the clang-abi-compat.cpp test.

In D122663#3457330, @erichkeane wrote:

LGTM! I would like @rjmccall to take a pass if he ends up having time in the next day or two (perhaps tack on an extra day or two because of Easter), else I'll be willing to approve later in the week.

Fixed release notes to use correct RST syntax.

Extend test, add assert.

Add to release notes.

In D122663#3457131, @erichkeane wrote:

Our most recent direction is to document any non-NFC patches in the release notes if at all sensible, so I think this meets those requirements.

In D122663#3456507, @erichkeane wrote:

I believe the answer here is 'yes'. We also likely need release notes.

Allow the old mangling with suitable -fclang-abi-compat= value

ping. Apologies, I don't know who to add as a reviewer, there is no one specifically listed as code owner and it does not seem to be handled by anyone in particular. @urnathan, @erichkeane, you two appear to be the most recent people who pushed commits specifically for mangling issues, would either of you be able to review this?

I've pushed D122540, would you like me to push this as well or would you rather do that yourself?

I mentioned that I'd prefer to get another person to review this as well but in the interest of just getting the bug fixed, I'll merge this tomorrow if there is no more feedback.

In D120254#3419369, @svenvh wrote:

In D120254#3419221, @hvdijk wrote:

The problem is that this change enables certain built-ins in OpenCL 1.2 that take a memory_scope argument, but the memory_scope type is not defined in OpenCL 1.2 mode. When we then process the function, sub_group_barrier in my example, things break when checking whether the declaration matches the built-in. I am not sure what the right fix here is. Can we just define the type if any extension is enabled that requires the type, or is that not allowed?

Thanks for digging further and providing a reproducer! I think the fix is to only make the sub_group_barrier(cl_mem_fence_flags flags, memory_scope) overload available for OpenCL 2.0 or above. That would also match opencl-c.h.

The following patch seems to fix the issue that you described:

In D120254#3342551, @dyung wrote:

Hi, our internal release build bots are showing failures in two clang-tidy tests that I bisected back to your commit, clang-tidy/checkers/altera-id-dependent-backward-branch.cpp and clang-tidy/checkers/altera-single-work-item-barrier.cpp. After this change, both are exhibiting this error:

Error while processing /home/dyung/src/upstream/aa9c2d19d9b73589d72114d6e0a4fb4ce42b922b-linux/tools/clang/tools/extra/test/clang-tidy/checkers/Output/altera-single-work-item-barrier.cpp.tmp.cpp.
error: enum type memory_scope not found; include the base header with -finclude-default-header [clang-diagnostic-error]

Oddly, this only fails in a release configuration. Can you take a look?

Question: does this need to be covered by -fclang-abi-compat= when the prior mangling resulted in names that even llvm-cxxfilt agreed made no sense? (If it is needed, it should be an easy change.)

In D122540#3413415, @RKSimon wrote:

The summary mentions fixes, but there are no extra tests to show the fix?

In D122540#3412980, @skan wrote:

I believe you can come up this patch independently. But I did make some suggestion about potential solutions in D103427 and prove it in D122537. Would you mind adding me as the co-author and reviewer?

In D118935#3399095, @Fznamznon wrote:

I was under impression that the change is small and therefore easy to understand. Would some comment like "SYCL re-uses OpenCL mode diagnostics to emit errors in case the cast happens between disjoint address spaces" help?

Hi, what is the rationale here? This reuses the logic that was written for OpenCL mode, but in OpenCL mode, it was made an error with the idea that a new keyword addrspace_cast could be used in those cases where the user actually wants an address space cast. Here, in SYCL mode, it's just made an error with no way out for the user that I can see if they actually want this. Is this really correct?

Simplify ProfiledCallGraphNode's types by reusing earlier types.

In D116832#3238011, @greened wrote:

In D116832#3236435, @pengfei wrote:

We have a huge issue with codegen churn causing pages of testcase changes in proposed merges and very little review is done on them.

Can you show an example?

Sure.

In D86310#3226142, @rnk wrote:

In D86310#2736983, @hvdijk wrote:

There is a risk of bitcode incompatibilities with this change, but we already have that the code we generate now is incompatible with GCC and results in crashes that way too, I don't think there's a perfect fix, I'd like it if we could merge this. I came up with roughly the same patch today, based on current sources, to fix bug #50198 before finding this one.

Who exactly generates GCC-incompatible code, clang, LLVM, or some other frontend? My understanding is that Clang handles most struct layout and alignment concerns in the frontend.

Apologies, forgot about this, and thanks. I personally think it might have been slightly simpler to check whether dot -Tpdf </dev/null (or an equivalent that avoids the shell) exits successfully, instead of having to parse dot's output, but since you already have it working this way, I'm also okay if you want to leave it as it is.

@tstellar Apologies, but I had very intentionally not reviewed this. This patch was misleadingly presented as doing what you had asked when it does the opposite (note the edit history on https://reviews.llvm.org/D96914#3141049, the author is perfectly aware that this is opt-out, not opt-in, but presents it as opt-in anyway), and was accompanied by an insult. I don't think there's actually any point in trying to engaging constructively here, the author is engaging in malicious behaviour and as long as that continues the technical aspects are irrelevant.

In D96914#3128270, @MaskRay wrote:

-Wreserved-identifier which is not in -Wall/-Wextra catches exactly this.

In D96914#3128155, @MaskRay wrote:

(I shall note that declaring reserved identifiers __start_ is UB in the first place. Well, the compiler has just always been just permissive.)

In D96914#3128137, @jrtc27 wrote:

What the warning suggests as a fix is entirely TBD; can suggest any and all of source, codegen and linker flag changes

In D96914#3128079, @jrtc27 wrote:

So spit out a warning that the default is going to change if we detect --gc-sections + start_/stop_ symbols + no explicit -z (no)start-stop-gc? That's probably a good idea to do regardless.

In D96914#3128046, @MaskRay wrote:

I can accept @hvdijk's "enabling -z start-stop-gc behavior for LLD 14" timeline, which only means a revert on release/13.x, without touching main.

In D96914#3127982, @tstellar wrote:

@jrtc27 @hvdijk Do you have a suggested timeline for transitioning to the new default?

In D96914#3115988, @MaskRay wrote:

In D96914#3115794, @tstellar wrote:

What are the downsides of reverting 6d2d3bd0a61f5fc7fd9f61f48bc30e9ca77cc619 ?

@tstellar Various metadata section users will get affected because they will see unneeded size increase.
Such users typically use Clang solely and use the main branch so if you keep the revert to release/13.x branch, that'll be fine.

In D96914#3110237, @jrtc27 wrote:

Yes, https://bugs.llvm.org/show_bug.cgi?id=52384, marked as blocking for 13.0.1 so a decision is made one way or the other before we unleash the "real" release on the world.

In D96914#3110224, @MaskRay wrote:

This is a version lockstep question. How well can an LLD of version X support llvm-project of version Y?