In D76909#1954500, @steven.zhang wrote:

LGTM on the power side changes, as no perf regression found with this change. I will leave others to accept this patch as I am not qualified to do.

LGTM, Thanks.

Looks good.

Nice one. LGTM

Thanks, much appreciated. And thanks for adding the extra test.

Looking good.

Yeah, the multiple uses are a pain.

Thanks.

The code looks OK. I think update_llc_test_checks should work, I've used it elsewhere in the past.

The code that already exists in the function you are changing is essentially already doing the same thing as you add here, just in a more constrained set of circumstances. It is saying that if the operands are obviously invertable, then the VPNOT will be really free and we can go ahead and invert the and to an or. The question is if that is true for all cases or not. For the test cases you have here it is true that the operands are easily invertable, but that won't be true for everything.

Oh yeah. Because we are now sinking float splats. Still LGTM.

Hello. Unfortunately we have some tests where it looks like this is needed. This code: https://godbolt.org/z/DpJuFm is no longer reaching the same minimum after this patch. Any ideas?

Hello. I like the idea. It's something we thought about internally but no-one has ever worked on enough to see how much of an improvement it gives in general.

We had these patterns before and took them out because they were not correct. My understanding is that these instructions do trunc(shift(add(sext(a), sext(b)), 1)). They internally operate in a higher bitwidth than we natively have.

In D76909#1946106, @jsji wrote:

Looks like this change has more impact to pre-P9 models, especially ppc32/ppc64, which we were using itineraries . (And I know there might be bugs in those itineraries)
Although some changes doesn't look good at first glance, we should dig into itineraries of each model to see whether this is problem in itineraries or due to this change.

@steven.zhang Can you help to look into some of them , especially for P6-P8 tests. Thanks.
@jhibbits FYI, you may want to check whether those changes in G3/SPE are good.

rGc9eaed514929

Tests look nice on this one.

In D76847#1945904, @samparker wrote:

I'm surprised not to see more test changes, does this really have no effect on any existing tests?

Unpredicatable -> Unpredictable

In D76910#1945744, @samparker wrote:

Are we talking about CONSTRAINED_UNPREDICTABLE here? If so, why is this modeled with hasSideEffects?

Added some tests for both top and bottom vmull's

LGTM

Unfortunately, I've written this pass in a single commit, so there is no easy way for me to split this patch in 2.
I can do it if you want, it's not impossible, but it's going to take me a while to get right. Also, the second optimisation is a relatively small part of this patch, so patch #1 would still be a large patch.

Is it possible to split this into two patches? The pass and "replaces VCMPs with VPNOTs when possible" part, then the second part to replace the re-use with the not. I think that would make each part easier to review, more manageable.

We added a VECTOR_REG_CAST, which it like a bitcast but doesn't change the bits. Similar to the AArch64 NVCAST.

SGTM

No longer a virtual method.

Amazing. I was going to say I'm not surprised we get big endian wrong. But it's a movimm. I am a little surprised.

I see. Because we are just swapping around the same values anyway. Makes sense.

I have altered the way that MVE VDUP are lowered, and we end up with a VMOV now instead of using a COPY. That means I don't need this any more for MVE at least.

Rebased onto the VDUP type changes.

Looks nice.

LGTM

Sounds great, from what I can see. The predicated lowering looks useful when/if we try and get predicated vecreduce's working.

I'm still not convinced that this shouldn't be done in ISel. There's nothing cross-block going on, so this is what ISel is designed for. It might make sense not to think of this as trying to convert vector_shuffle to something else, but instead trying to convert what vector_shuffle has turned into into something more optimal. In that one case I looked at, there was something like a (v4i32 (ext (v2i32 (buildvector (v4i32,..))). We get this way because a v2i16 was legalised to a v2i32, but everything around it was a v4i32. Can we "flatten" the ext into a single BUILDVECTOR? I have not had time to see if that is or isn't possible, but it sounds more sensible than very special case pre-isel legalisation for certain shuffle_vector's.

In D76292#1926983, @efriedma wrote:

I haven't been following the MVE work that closely, but changing the operand type of MVE vdup makes sense. My one concern here is the potential for confusion due to the opcode; VDUP for NEON and MVE have the same opcode and result type, but the operand types are different. Doesn't really matter much for isel patterns, but could be confusing for writing target-specific combines.

LGTM

Now using TargetRegisterInfo::shareSameRegisterFile.

Thanks for the suggestion.

In D76024#1918460, @efriedma wrote:

I'm not sure what, exactly, is specific to Cortex-M here. Copies between int and SIMD registers aren't really cheaper on other CPUs, relatively speaking. I don't really like adding a new target hook without a better justification for why Cortex-M is different from other targets; having target-specific codepaths is going to make it harder for anyone to make improvements here in the future.

Thanks

In D76023#1918962, @samparker wrote:

Annoying about the vmovs.... I can't see, with register aliasing, how this codegen wouldn't be a regression.

Good to see this getting updated.