In D72961#1827862, @efriedma wrote:

My initial reaction was that the PHI was invalid, but according to @qcolombet it's ok to have different register banks on the operands

Okay... but the operand register class can't actually be *anything*, can it? We have restrictions on COPY instructions.

Those restrictions are target specific though aren’t they? I think it’s the responsibility of ISel to generate valid copies and similar should apply to PHIs.

In D72961#1827624, @efriedma wrote:

What is the restriction on the register classes of PHI operands, if they don't have to have to be same register class as the result? I don't see any relevant documentation, or checks in MachineVerifier::checkPHIOps().

LGTM.

I think in cases where it's possible to use an fmov with immediate:

Re-closing.

Fixed in 2e39ea726e9.

Sorry about that. I've pushed a fixed version in 2e39ea726e9.

LGTM.

LGTM.

Ping.

On the topic of this particular change: IIRC the semantics of bitcast is that it's equivalent of doing a store of the source type and a load of the dest type, which can have differing results on big endian targets. Is this lowering code safe in that case?

In D72492#1814698, @arsenm wrote:

In D72492#1814681, @aemerson wrote:

In D72492#1814675, @arsenm wrote:

In D72492#1814575, @arsenm wrote:

In D72492#1814528, @aemerson wrote:

On AArch64 we also use bitcast to convert from <n x p0> to <n x s64> types.

I thought that was illegal. It’s not legal in IR

I think this is a verifier bug. You're not allowed to bitcast between scalar pointers and scalars, so allowing it for vectors doesn't make much sense. I think allowing this is potentially problematic for non-integral pointers since it makes it easy to accidentally launder away the pointerness

We don't have an alternative way to easily convert between them though. Unless G_PTRTOINT is also allowed to take vectors of pointers.

G_PTRTOINT works with vectors. I'm not opposed to allowing G_BITCAST between scalar and (integral) pointers, although this differs from the IR. It makes more sense in codegen to have these be more flexible. The current state doesn't make sense though. We can either:

Ah, I didn't realize it was allowed in the IR. In that case AArch64 can move to using that instead.

1. Allow casts between pointers and others types. This will need documenting as differing from the corresponding IR instructions. This should also maybe restrict this for nonintegral address spaces?

1. Disallow bitcast between vectors of pointers and vectors of scalars and match the IR

In D72492#1814675, @arsenm wrote:

In D72492#1814575, @arsenm wrote:

In D72492#1814528, @aemerson wrote:

On AArch64 we also use bitcast to convert from <n x p0> to <n x s64> types.

I thought that was illegal. It’s not legal in IR

I think this is a verifier bug. You're not allowed to bitcast between scalar pointers and scalars, so allowing it for vectors doesn't make much sense. I think allowing this is potentially problematic for non-integral pointers since it makes it easy to accidentally launder away the pointerness

On AArch64 we also use bitcast to convert from <n x p0> to <n x s64> types.

No objections for a week, LGTM.

Add asserts.

LGTM.

LGTM with some minor comments.

LGTM.

Use getConstantVRegValWithLookThrough.

Since you're introducing a new opcode for this, can't we implement a Lower action? Daniel mentioned a while ago that he also prefers having targets explicitly opt-in to using default lowerings/expansions.

LGTM.

Looks reasonable to me.

Update patch to use !isSimple() instead of isVolatile().

In D70465#1776252, @rnk wrote:

Any thoughts on this?

In D71153#1773787, @saugustine wrote:

Committed aa3c877fb58db238a86e734511bdeed9bc40086c to fix the unused variable warning at AArch64InstructionSelector.cpp:1013

Thanks!

In D70794#1768237, @drodriguez wrote:

@aemerson: Thank you very much for accepting. I think I reduced the test case correctly. I needed to use the result of the G_LOAD, or the code was not being generated with the necessary G_TRUNC. I didn't imagine that it was as easy as returning the result. I hope the new test is better than the last.

Also, I don't have permission to merge, what's the next step for me to do? Thanks.

I can commit it for you later, after a few patches you can request commit access for yourself.

I think the test can be simplified a bit to not have the extra blocks and just testing the relevant instructions, otherwise LGTM.

Could you upload a diff with more context (-U9999)?

Adding @craig.topper and @RKSimon to weigh in.

Is a compiler abort not preferable to generating non-ABI conforming code?

In D42512#1455303, @jyknight wrote:

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.

Additionally, this patch unnecessarily breaks calling vararg functions with _integer_ arguments, which in particular, the EDK2 project does, with windows ABI functions and sse disabled. So, since this breaks existing working code, I'm going to revert this change now, and then propose a different change to address the crash.

In D63973#1751315, @jonpa wrote:

I'm inclined to xfail those tests and open bugs for them

Like this?

LGTM.

Should be fixed in 0f6ed432d58e47e7082bfd44d7b29f3ee54e2642, thanks for the report!

LGTM with nit.

In D68151#1686689, @arsenm wrote:

In D68151#1686687, @arsenm wrote:

I'm somewhat worried about this wrt. eventually verifying intrinsics have the correct number of operands

At minimum the verifier should manually check this operand and manually verify these intrinsics

I'm surprised this wasn't covered by an existing test?

In D67133#1679497, @aditya_nandakumar wrote:

In D67133#1677499, @aemerson wrote:

Ping. Can we combine the delegate and observer installers?

Thanks Amara for the feedback. The reason I haven't responded here (or committed this) is that I haven't had the chance to track and fix the issues in the various backends where this fails (probably missing Observer calls). Maybe for now, I will disable the verification by default and get this patch in, and we can work on enabling it by default in a follow up patch?

Ping. Can we combine the delegate and observer installers?