Advertisement

Close

I approve. Thanks George!

In D98797#2634898, @lattner wrote:

Advertisement

Close

This is fantastic! I'd recommend mentioning this on llvm-dev / and to Alex @ llvm-weekly when it goes up.

Advertisement

Close

Addressing feedback + also adding a pointer to the various flang and openmp regular sync-ups.

Make capitalization consistent

Thanks Chris.
Now updated following all of your suggestions.

LGTM, thanks!

The rendered patch looks like this:

Thanks for this patch Oliver!
I've started reviewing, but am not very far yet.
I thought I'd share my thoughts so far already, as I'm not sure exactly when I'll be able to do a complete review.

Still LGTM

LGTM, thanks!

In D90868#2613237, @rjmccall wrote:

Before we get too far into editorial review, I think we should step back and ask what actually needs to be in this document. In particular, I'm not sure that the discussion of how pointer authentication can be used in an ABI is really appropriate for LLVM-level documentation. We should discuss the formal model we want the intrinsics/constant to provide — secret key registers, well-formed pointers, arbitrary discriminators — and just link to other documentation (e.g. the much longer white paper in the clang docs) for the benefit of people who are curious about how this can be used.

In D96164#2547858, @gkm wrote:

df>>! In D96164#2547480, @smeenai wrote:

In D96164#2547390, @thakis wrote:

LLVM tries to not depend on external libraries if possible. Given that sha256 isn't that much code (https://golang.org/src/crypto/sha256/), just adding a fresh implementation of it (to lld/MachO for now) is probably the way to go.

Agreed. There's some prior art here too ... LLVM has a SHA1 implementation in llvm/include/llvm/Support/SHA1.h and llvm/lib/Support/SHA1.cpp. That was adapted from an existing public-domain implementation, and we could do the same here (find a SHA256 implementation with a license that allows it to copied into LLVM).

Openssl has a C implementation of sha256 with Apache 2.0 license. Unless someone knows of a better candidate, I'll go with that.

In D96482#2556315, @tschuett wrote:

If you would extend the paragraph, you could say:
"For reference the SPDX identifier is ..."

LGTM, thanks!

Just a few quick, knee-jerk-style, questions. My apologies if these are too simplistic or vague.

• Do you happen to have a pointer to some more documentation about this new Swift feature?
• I wonder if any features from other languages could also map to this IR/backend implementation? E.g. I haven't looked at how C++ co-routines are implemented, but would they be able to reuse this feature (assuming they would need a somewhat similar feature in mid- and back-end).
• As this seems to be introducing new ABI - do you have an idea of what the most appropriate ABI specification is this would be best documented in? Do I guess correctly that this is not a Darwin-only feature? If this is really swift-specific (i.e. it does not seem to be a good trade-off to have this mid/back-end feature support similar language features for multiple front-ends), I guess this is Swift-specific ABI. Do I guess correctly that there isn't really a specification for Swift-specific ABI - it's purely defined by its primary implementation?
• I just had one very detailed nit-pick comment on the documentation of frame layout - see specific comment.

My understanding is that AArch64 gnu_ilp32 support is present in mainline gcc, but not in mainline glibc nor in the mainline linux kernel. My understanding is also that it's unlikely that AArch64 gnu_ilp32 support will be added to mainline glibc or linux kernel anytime soon.
That makes me wonder how useful it is to support AArch64 gnu_ilp32 in mainline LLVM. It seems unlikely that it can be used for linux targets anytime soon. Are there non-linux targets using AArch64 gnu_ilp32?

In D93221#2459903, @ostannard wrote:

Why is this restricted to v7-A or later? The DSB and ISB instructions have existed since v6T2 and v6M.

Updated patch based on review comments + rebased to ToT

Updated patch based on review comments + rebased to ToT

Updated patch based on review feedback.

LGTM too, thanks!

Thanks for this.
The proposed changes all LGTM.
I did do a grep on the term 'master' in llvm/docs, and found quite a few more uses of 'master', especially in links.
The command I used was: grep -R master llvm/docs/
Just two examples are:

In D92328#2423327, @george.burgess.iv wrote:

thanks for this!

i think the preferred way to PGO LLVM is cmake: https://llvm.org/docs/AdvancedBuilds.html

that said, while we have this script & the docs pointing to it, seems good to keep it up-to-date. LGTM

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!