In D89068#2326910, @pietroalbini wrote:

This seems to be approved, what's the process of landing the commit? I don't have commit access to LLVM.

Thanks for this @pietroalbini !
This LGTM.
I'd also like to hear @mattdr's or @jfb's opinion before committing though.

LGTM, thanks!

This looks fine to me; I just have a number of nit picks.
The only part where I don't understand the code logic is around the comment starting with "If this is the final byte of a multi-byte sequence".

In D81403#2097148, @zbrid wrote:

LGTM

Could you explain how not adding the MBB entry block is different from removing extra MBB later? Curious on the details about the differences here since the change makes sense functionally and I don't see any issues, but I can't understand why adding the extra MBB entry block at the point we create the machine function and then deleting extra blocks results in something different than the automatically created entry block. Is it some attributes or configuration in the basic block or something?

Updated version based on @ostannard feedback.

This new revision fixes the root cause of GlobalISel asserting on an IndirectThunk: the IndirectThunk creation code was producing a MachineFunction with a slightly different structure than what gets produced from a naked function implemented in C code and processed by clang.
This fixes this by letting the IndirectThunk generator produce a MachineFunction structure that is the same as what gets produced by clang for an empty naked function written in C.

Based on the review feedback that using X16 or X17 could be problematic since a linker can clobber those registers on a call, this patch goes for a different approach to achieve compatibility with BTI-enabled callees.
It simply moves the register to be called always to be in X16, and then does BR X16.
It seems it's simplest to do that unconditionally, as it may not always be easy to find out if the call target has been BTI-enabled.

The updated patch addresses Oliver's review comments.
Especially, it addresses the review feedback regarding potential clobbering of X16 or X17 when the call to the thunks go through a long veneer.
It addresses this by avoiding generating indirect calls that use X16 or X17.

Thanks for the feedback!

I ended up pushing a fix for the racy tests in 7e6f891df85

I ended up pushing a fix for the racy tests in 7e6f891df85

I think I'd like the proposed organization better; but I only have a weak preference.
LGTM.

LGTM

Assuming there is no change in output, LGTM.

This seems OK to me.
I'd just remark that these days, I think most people using LNT to compile/run/benchmark test-suite use "lnt runtest test-suite" rather than "lnt runtest nt".
Therefore, maybe it'd be better if you'd look into how to make that working on the VE architecture (i.e. look at file lnt/tests/test_suite.py).

LGTM. Thanks!

In D74918#1923151, @zoecarver wrote:

There are a lot of different ways we could implement the feature. We may want to only enable it when -march=native, or maybe only in the unstable ABI, and maybe we want to support aligned pairs on some architectures. I think that's an important discussion to have but I'm not sure _this_ patch is the best place to have that discussion.

Even if we don't use this patch in the implementation I think it would still be a good utility to have. Here's what I suggest: I commit this, create another patch to add a builtin that exposes this API, and then open a libc++ patch with a _possible_ implementation. In that patch, we can discuss how we should actually implement the feature and after we have a consensus I can do the work to implement it. Any objections to that plan?

In D74918#1898191, @__simt__ wrote:

In D74918#1897636, @kristof.beyls wrote:

If these values are part of the C++ target platform ABI, it seems to me the values for std::hardware_{constructive,destructive}_interference_size should be set by whoever has the authority to decide C++ platform ABI for specific platforms.
Assuming my thought in the previous sentence is correct; discussions on which values to chose for std::hardware_{constructive,destructive}_interference_size should happen in whichever forums decide C++ platform ABI for the various platforms? (Maybe for some platforms that forum might be clang-related fora like reviews.llvm.org, but probably not for all platforms).
With my (probably limited) understanding of the requirements, it seems like deriving std::hardware_{constructive,destructive}_interference_size from actual cache line size on a specific micro-architecture doesn't seem to be the right approach?

They will be in the library ABI, meaning the libc++ ABI.

It's valid for libstdc++ and libc++ to have different values there. If we wish, we could try for an alignment (no pun intended) on these values, but even then that's just between these two libraries.

Which is good and encouraging, because I don't know what forum we would have to go to.

In D74918#1896930, @__simt__ wrote:

(I assume I'm not seeing a code review being used to veto a C++ Standard feature, but actually the other points are the reason for the red flag.)

I can see a desire for hyper-precise definitions to achieve the best possible performance, but we really need a healthy does of conservatism here.

The C++ values can't change as a result of selecting between microarchitecture variations that are supposed to link, it takes an ABI break to change these.

As someone who isn't up to speed on the design of vector-function-abi-variant, this release note makes sense to me.
It'd probably be best though for this to be approved by someone who understands the design.

After chatting with Oliver, he states that he did run ubsan before and after this change, and the undefined behaviour that was causing the segfaults is gone with this change.
(There are apparently other undefined behaviours flagged by ubsan, but they are unrelated).
I also don't know much about H264 decoding, but the removal of undefined behaviour as indicated by ubsan together with no change in output makes me believe this change is good enough for how the test-suite is used.
LGTM.

In D73070#1834196, @nickdesaulniers wrote:

I don't mind landing this without aarch64 bti interplay matching GCC precisely; I think it can be done in a follow up. I just request we don't try to land this in the clang-10 release unless that is also resolved. Otherwise clang-11.

Addressed Sander's comments.

I agree that the disabling of emission of stack maps in non-MachO AArch64 objects in rL206610 looks like an accidental mistake.

I agree the bare names could cause a lot of confusion, and that the naming scheme proposed in this patch resolves that potential confusion.

IIUC, I think "isAuthenticating" would be a more accurate description/name than "isAuthenticated".
In that the instruction on which this property is set does authenticate a pointer, rather than the instruction itself having been authenticated (whatever that may mean) before.
Does that seem fair?

Would it still make sense to have this patch after D68683 lands? At first sight, it seems this patch might no longer make sense then?

LGTM, thanks!

Thanks for persevering Sam.

Given this seems to add a new IR instruction, shouldn't there also be good quality documentation for this new instruction in docs/LangRef.rst?

In D67485#1676748, @t.p.northover wrote:

Enabling the corresponding subtarget feature on cyclone doesn't seem safe to me. If we ever implement -mtune, then a command line like clang -march=armv8a -mtune=cyclone should mean “generated correct code for the v8.0 architecture, but optimize for cyclone". Adding this feature to cyclone as is would probably result in the above command line producing code that isn’t architecturally correct for v8.0.

I don't think Clang really does anything with -mtune yet. The most it could do based on the way CPUs are implemented in LLVM now would be something like applying the scheduling model. Almost all of the features in the list are going to break older CPUs.

Thanks Sam, I think this is starting to look good!
I just had a few more nit-picky comments.

LGTM

I'm afraid I'm not up to speed on statepoints, but I thought I'd give some feedback nonetheless on the code as is.
I hope it's useful.

I think you also need to call out the new SingleSource/Regression/C/gcc-c-torture directory in the top-level LICENSE.TXT file as a directory containing non-LLVM licensed material (see bottom of the top-level LICENSE.TXT file).