Thanks @srhines @javed.absar, LGTM!

@srhines: Do you need Android to *always* be 64-bit, even if someone forces aapcs with it?

I don't see why we should force the target, given that this is a CodeGen test, should worr (and be tested) on all targets.

Just FYI, when you submit a patch, use "git diff -U999" so that we can see the context in the web interface. Makes it easier to review. :)

Where is this failing?

Makes sense to me. But now that "generic" is the default, it'll impact all cores equally, so I'll let other people comment before approving.

In D32166#768590, @jonpa wrote:

I have just looked at performance. It was a while since I tried to evaluate the compile time impact of a single pass. How would you do this?

In D32166#768530, @jonpa wrote:

I tried this - adding LSR just before loop vectorizer, and found that it did indeed affect a few benchmarks noticeably, with mixed results.

A few more comments... :)

Hi, can you please re-share the patch with more context?

Silly inline comment, but otherwise, LGTM. Thanks!

Ok, I think it's good as it is. Looking forward to more patches in this area. :)

Approving, so that when Steve does the same, the review is free to commit.

In D33205#762066, @javed.absar wrote:

Hi all:
I have made the changes (excluding Android) as recommended. Please have a look if it is now fine.

Hi Javed,

Some initial comments...

In D32166#745689, @jonpa wrote:

In this sense, isn't the loop vectorizer run too early?

In D33205#761141, @olista01 wrote:

Ah, I just found the email thread, looks like it never reached phabricator. Sorry for the confusion.

Wait, Stephen had a concern, we need his feedback on my reply before approving.

Hi Ayal, sorry for the delay. Can you commit the plan doc separately?

In D32737#760473, @chandlerc wrote:

Put differently, this patch makes sense *once* we clearly have consensus on llvm-dev. So far, the only thread I can find did not reach any meaningful consensus. Notably, none of the code generator people were heavily contributing to that thread, and there remain large unmentioned technical concerns (stack spills, alloca size, etc)

In D32737#760457, @hfinkel wrote:

As I explained to Hal on his comment, that is correct but doesn't have the effect you're expecting.

Which comment? FWIW, I didn't see a particular response.

Just to be clear, I'm not *against* the idea of an intrinsic, nor I'm pushing this patch for any personal/professional agenda. I hope I have made that perfectly clear on my previous reviews on the same patches before.

In D32737#760430, @lattner wrote:

From my point of view, making scalable vectors a native and core type of IR is the only way forward, because the semantics needs to be ingrained in the language to make sense. AFAICS, at the IR level, the differences between vectors and scalable ones is not that big a deal, certainly not bigger than vectors versus scalar.

This is much more up for debate. Details matter. Compilers are a set of engineering tradeoffs. You need to explain and defend your position carefully, not just state it as though it were "obviously true".

In D32737#760310, @sanjoy wrote:

I've only lightly read the spec, but it looks like the vector length can be controlled by writing to the ZCR_ELn registers (so, e.g. user code could make a syscall to change the vector length)?

In D32737#760289, @lattner wrote:

I agree with Chandler on this. Making something a first class type in LLVM has wide reaching effects, including requiring the ability to load/store/phi the value, pass it by argument, etc.

In D32737#760276, @chandlerc wrote:

Uh, where was it discussed? It's entirely possible I missed it, but I can't find any consensus on any of the threads that we actually want to support runtime vector width in LLVM's IR.

Right, I think this is a useful thing to have, but it needs a better plan.

@echristo @chandlerc @lattner @majnemer Ping.

Hi Sjoerd,

Hi Javed,

In D32422#757432, @jonpa wrote:

Right, I see what you mean about the "address computation" to be forced into scalar registers. Does that mean that you have to use the same GR to load into all lanes of the vector, so supposedly load+increment+load+increment...?

It doesn't have to be the same GR for all the lanes, but it must be a GR (since we don't use the slow "element gather/scatter")

Regardless, between iterations of the loop, you want to keep the addresses in the GRs throughout the execution of the loop. Is that correct?

Yes, that's better than having to extract elements. The extracts are relatively expensive.

In D32422#757376, @jonpa wrote:

I don't quite follow how this has to do with vector shuffles...? On SystemZ, all addresses residing in vectors must be extracted before use. (There are slow vector element gather/scatter, which are not used by the backend, so they are irrelevant).

Hi Jonas,

Perfect, thanks! LGTM.

Right, just to make sure it's doing what we expect it to, did you try to run this with targets "armv7a" and "armv4t"? They should have different results.

Hi Weiming,

As parsers go, simplicity and elegance are usually mutually exclusive.

In D32605#750202, @joerg wrote:

That's not true. ARMv4 for example has no clz, it's a V5T feature.

In D32605#740044, @joerg wrote:

I agree. If the CPU has it, it will be beneficial. If it doesn't, it is only a useful transformation if the intrinsic can be constant folded.

In D31801#750077, @steleman wrote:

Ping2!! :-)

Wait, we also need the TTI callbacks. I thought you were going to introduce them.

In D30086#748611, @aemerson wrote:

When people add new intrinsics they don't really add tests to check the IR since you get assertion failures during the FunctionType creation if something goes wrong.

Right, this is looking much better. Now, what about tests?

Hi Amara,

Also, where would this be used?

Sorry for the delay, LGTM. Thanks!

@echristo, any comments on this?

Right, makes sense. This is an obvious fix, so I feel comfortable approving this one. LGTM. Thanks!

Wouldn't it be better to set the type manually to something like uint32_t or uint64_t? To make sure they're the same across all arches?

LGTM. Thanks!

I'm ok with the change, as long as @javed.absar is happy. :)

I really thought it was there already! Thanks for the patch! LGTM.

You seem to have lost the negative test.

With that in mind, I think you should then go back to your original implementation (ignore quotes) and let the expression evaluation crash if there's any single quote in between (or whatever is the expected behaviour).

Hi Graham,

Hi Jonas,

In D30086#743350, @aemerson wrote:

Ok, so I've restructured the two functions a bit so that the simple (non minmax) reductions are generated from the createSimpleTargetReduction() function and the recurrence descriptor uses that for the simple cases, passing in the opcode.

Thanks! LGTM with the test change.

Can you merge D32523 with this one?

In D32605#740840, @evstupac wrote:

Yes.

In D31801#740740, @steleman wrote:

So, then, this would be a case of "some micro-architectures can do this for any floating-point type, some others need special treatment".

Right, I clicked the link and it took me to the old code. :)

Hi Ayal,

In D32460#740135, @majnemer wrote:

In D32460#740134, @compnerd wrote:

Yes, the idea is that the textual IR uses the correct triple, unless it is overidden at the command line.

Why not just handle this in clang?

In D32605#740041, @evstupac wrote:

Guarding is easier. Let's do this only if someone complains about performance.

In D32605#739888, @evstupac wrote:

In this case builtin should be expanded at codegen in the most profitable way.

So, I think the document is so good and can be very helpful in further communications, that I think it should be committed separately.

Hi Evgeny,

Hi Graham, thanks for this work.

I think this patch and D32523 are small and simple enough that it could be merged into one...

Adding Tim, as this is likely going to affect Darwin more than Linux.