Please just merge the two classes. There is no good reason at the moment to separate them.

Abandoning this in favor of removing ManagedStatic entirely via the stack of changes that ends in D129134

Abandoning this in favor of a stack of changes including D129129

LGTM, one suggestion though.

What happens when the register allocator decides to split a live range of virtual registers here, i.e. if it introduces a COPY?

LGTM

In D125795#3599173, @nikic wrote:

In D125795#3598460, @nhaehnle wrote:

I'm in favor of doing this, and the change looks mostly straightforward. However, I wonder if, since it is a breaking change in the C ABI, we shouldn't have at least one release cycle where LLVMConstExtractValue (and other functions that are planned to be removed) are marked as deprecated.

I don't think that's necessary. Per https://llvm.org/docs/DeveloperPolicy.html#c-api-changes C API stability is "best effort", presumably exactly for cases like this (the underlying functionality that is being exported simply no longer exists). I don't think unavoidable changes to the C API should hold back changes, especially as the impact on the C and C++ APIs is the same here. Of course, we should have a release note for the change, which I have now added.

Thanks. This looks pretty good to me, but I'm not familiar with the bitcode reader so perhaps somebody else can still take a look.

Awesome, LGTM

Thank you. One more thought after sleeping on it: What do you think about !exists? I think !exists<T>("foo") reads nicely as "does there exist a record of type T with name 'foo'?". It's not perfect, but I like that there's less confusion with "instanceof" or "isinstance" operators that some programming languages have.

I'm in favor of doing this, and the change looks mostly straightforward. However, I wonder if, since it is a breaking change in the C ABI, we shouldn't have at least one release cycle where LLVMConstExtractValue (and other functions that are planned to be removed) are marked as deprecated.

LGTM

Most of this looks good to me, I just have a few nits and one bigger question about phi handling inline.

Can this be abandoned in favor of the !instanceof change?

Hmm, I now saw the earlier discussion and @tra's very good points in it. If !instanceof accepts defs, then it becomes somewhat redundant with !isa, which I hadn't considered. Can you improve the error checking so that if a record is provided as an argument, the message advises that !isa should be used instead?

I think this change makes sense and it looks good to me, except for one thing: if I were to see an !instanceof without context, I would expect that it is also able to take a record/def as operand. That kind of use is likely to come up eventually, and people shouldn't have to artificial convert the record/def to its name first. So I'm in favor of this change, except I'd ask you to change the implementation (and tests) to support both kinds of use.

Thank you for doing this. It looks alright to me.

Ping

Ping -- are there any substantial comments left? Based on previous feedback this is probably good to go with the changes I made last week?

Add a simple test case

Address reviews:

• fix comments
• moved to ArrayRefTest.cpp
• add a check that the mutable array ref elements can be assigned to

In D126411#3538893, @arichardson wrote:

This looks reasonable to me, would it be possible to add a test showing the new behaviour?

You can use the "Edit Related Revisions" option in the right-hand side menu of Phabricator to link this revision with the others of the series. I can't really speak for the Clang parts, but the LLVM parts looks reasonable to me modulo some detail comments.

It makes sense to unify that handling, but I object to the term "lane VGPRs". In a way, it's the other way around: the registers that are used for SGPR->VGPR spills are de facto "WWM VGPRs", because they're used in a wave-wide manner, which is the opposite of lane-based usage.

I don't think the premise of this patch is correct. A source-level writelane intrinsic is not allowed to overwrite inactive lanes. If it does, then that's UB. If you want wave-wide writelane in source, the way to achieve that would be to use WWM.

+1 for creating this stuff using TableGen.

Running TableGen multiple times is a bit annoying for LLVM build times. Have you considered packing all outputs into the same .inc file?

In D123901#3455711, @lattner wrote:

I'd prefer we have one thing. Re:

I like this idea - I originally had dyn_cast_or_null with an overload for Optional but it looked goofy. I like dyn_cast_if_present with maybe some forwarding dyn_cast_or_null to avoid breaking everyone.

it is super awesome to stage things in and give people a migration window, but if it is the right thing to rename this, then we should drop the old name, not carry it around forever.

+1 for doing this, thank you!

Thank you for doing this! Not a detailed review but the interface and approach LGTM.

I'm sorry to say that this patch seems confused about semantics. It lacks clear definitions, and in particular for the shuffle attribute in LLVM IR, you almost certainly just want the already existing convergent instead.

• address review comments
• build on a factored-out buildReadFirstLane (see D125324)

Having gone over the argument with @ruiling separately, I think he has a point. Have you double-checked whether this change is still required after the related LLPC changes have been made?

softwqm is used by our implementation of subgroup operations: if helper invocations are there, then subgroup operations must interact with them.

Thank you for the summary.

In D124884#3490649, @arsenm wrote:

In D124884#3490643, @arsenm wrote:

In D124884#3489967, @rampitec wrote:

To confirm: is that OK to add yet another imm to the end of operands of the intrinsic to select a byte size? And then remove the overload. If yes I will do it tomorrow.

There's the elementtype attribute for this case which some arm intrinsics seem to be using. Not sure how you're supposed to define an intrinsic to use it though

Apparently this isn't well developed but works. The verifier is hardcoding these intrinsics (it's also looking at the call site instead of the intrinsic declaration attributes)