With one bonus question, are the fused operations fast on the M7..?

Oops, sorry, forgot to reply, and also got distracted by a few other things. I ran one bigger benchmark, and didn't see anything worth mentioning. A first preliminary conclusion: looks like we don't miss much by not doing this lowering here. Disclaimer: I've tested only on Arm targets, definitely not on all interesting architecture combinations, and a handful of benchmarks.

Would it be worth adding a test for the M7 though? We seem to be a little lacking in our m-class FP tests.

should the f64 version not also be tested?

I think moving VFP4 check into the useFPVMLx method would help make this easier to read.

Looks fine to me

I am doing some experiments with a hack that simply comments out the call to InstCombiner::SimplifyAnyMemTransfer. I've ran 3 smaller benchmarks. 2 didn't show any difference. The 3rd shows 1 tiny regression and 1 tiny improvement in a test case, and will probably not even show a difference in the geomean. I am doing this as a background task; tomorrow I will ran bigger benchmarks, on different platforms. But I guess we need some numbers for non-Arm platforms too.

Thanks!

The ultimate goal would be to simply always canonicalize to memcpy and not expand it ever in instcombine as mentioned in D52081.

@efriedma : apologies for the ping, but does this look reasonable?

Looks okay to me.

Thanks for clarifying! Looks good, with only a nit.

LGTM

LGTM

LGTM

LGTM

Looks okay to me.

Looks okay to me.

Looks okay to me

Yep, sounds good, cheers.

Added FIXMEs, like in D50229, that this needs reimplementation too after the TargerParser rewrite.

Thanks for the reviews.
Now take code size into account, and removed outdated comment.

Added FIXMEs.

Ah, and just for your info, the proposal was just sent to the dev list:
http://lists.llvm.org/pipermail/llvm-dev/2018-September/126346.html

(I am now picking this up, and will try to progress this patch and also D50179)

Ah, but looking a bit closer now, I am not sure this is the right thing to do. This changes makes any 8 bit value cheap, including negative numbers. And I am not sure if this is the right thing to do, since the Thumb1 immediates are positive numbers. It looks this a workaround for store-merging interacting badly with constant hoisting.

I think the test file can use a bit of clean up: we don't the attributes, metadata, etc. But more importantly, can it perhaps be further reduced? Do we need all this code?

Good point. I wanted to worry about that later in a follow up patch, but perhaps that doesn't make sense. I will fix it now.

Reshuffled the tests a bit.

This is done better in D35035 (and is no longer stalled).

LGTM

Looks like a straightforward addition/cleanup to me.

I would quickly like to check the status of this patch: do you have plans to continue this work? If not, I would like to pick it up.

Cool, I am going to pick that one up then. Cheers.

One more thought then on this:

Do keep in mind that there is more than one backend, more than one target architecture.

Thanks for the feedback and suggestions! Summarising where we are:

• I think this WIP patch generates the code that we want, for 32 bit and 64. In the 64-bit backends, the 8 byte memcpy is expandend to just a load and store (see also comments below about the backend dealing with the memcpy). So what I said earlier, that we also generate the libcall for X86 and AArch64 with -Oz, this wasn't true due to a problem in my test.
• But the main problem now is that we are not happy with the current implementation.

Thanks both, those are fair points.

fixed the test case.

Lgtm

LGTM

Looks like a reasonable clean up to me.

LGTM

That's a cool new trick!

Looks like a straightforward addition to me.

We've had the pass enabled downstream for a couple of weeks and it seems to be okay, so enable it by default.

This looks good to me, but it's not really my area of expertise, so would appreciate if someone else can have a look too. Perhaps @olista01 or @efriedma ?

Looks reasonable to me.

Looks like a straightforward addition to me. Cheers!

Looks like a straightforward fix to me.

Thanks, looks good to me.

LGTM

Looks okay to me.

Looks like a straightforward fix to me.

Looks like a straightforward fix to me.

LGTM

Looks okay to me

Nit picking the subject/description:

Thanks for reviewing.

These changes look reasonable to me.

Looks like a straight forward fix to me now.

fixed typo