Hi Martin,

I'm happy with the patch, but I'll let @SjoerdMeijer approve.

This change seems to have nothing to do with adding core support but exposing features from CPU names.

In D36814#844631, @return wrote:

This patch is needed in-tree for upstreaming my Swift port for Haiku, which is part of my Google Summer of Code project.

The swift-llvm fork regularly pulls changes from LLVM master into the upstream-with-swift branch, so adding this patch here should make sense.

Curiosity: Do you have a use for this in-tree or just off-tree?

Ok, LGTM. Thanks!

Adding Eric, for consistency.

NFC. LGTM. Thanks!

Other than the one comment, LGTM. Thanks!

Submit as a different revision and we can discuss the merits there.

LGTM, thanks!

Hi Oliver,

Yup. LGTM. Thanks!

There are two patches here, please split them.

ping @compnerd

Sorry, long holidays... :)

This makes sense to me. LGTM. Thanks!

LGTM too. Thanks!

In D35568#818708, @fhahn wrote:

Using 8 byte alignment gives a 0.25% speedup on execution time (was 0.23% with 16 bytes), a 0.82% improvement
in benchmark scores (was 0.93% with 16 bytes) and a 0.20% increase in binary size (was 0.55%). So for the score related benchmarks, the 8 byte alignment makes things worse quite a bit, but the impact on size is much smaller. Should we use 8 byte alignment, to keep the binary size down?

So, it seems it was sphinx, but that was loop alignment, 4 bytes on A53, 8 bytes on A57, to do with the fetch alignment. Maybe this is a related issue. Why 16, though?

We found similar results on spec2k6 for aarch64 that we attributed to function alignment. Have you tried that? I need to dig the one culprit...

In D35569#816129, @fhahn wrote:

With FeatureARM, the generic CPU would have to have this feature, but when the features of the architecture (v6m) are applied, FeatureARM cannot be removed, so the behavior will be different to the current one (Architecture features are applied after CPU features I think). We could rename FeatureARM to FeatureThumbOnly, which is a bit clearer in my opinion.

Apologies, you had already sent all I needed. :)

Hi Ayal,

In D35498#811867, @mkuper wrote:

typedef struct CvNode1D
{
    float val;
    struct CvNode1D *next;
}
CvNode1D;

And we're trying to vectorize code that loads these structs. Since the pointer and the float have the same width, we can load four of the structs as, e.g. 2 * <4 x float>, and then use shuffles to get a vector of 4 floats and a vectors of 4 pointers.

Setting aside from the fact that no one should ever be casting floating point to pointers, perhaps we shouldn't be even trying to do this transformation here, as I'm not sure this could have a guaranteed semantics in this case, and probably better to just bail the vectorisation?

In D35307#807706, @aemerson wrote:

This seems a very raw change, without any further description, comments or proper usage, other than a few changes on random places.

I don't agree. The additions are in line with the level of documentation in the rest of the file.

The Clang counterpart was approved, this looks good, too. Thanks!

Thanks. LGTM.

In D35118#807712, @aemerson wrote:

In D35118#806730, @rengolin wrote:

@jmolloy Can you check this change, please?

I'm not really removing FPUMode, I'm just converting an enum to a bit field. FPUMode, i.e. no NEON, after this change is now represented by simply having all bits be 0. See the equivalent implementation for ARM which does the same thing.

@jmolloy Can you check this change, please?

Hi Amara,

In D35076#801480, @aemerson wrote:

Yes I have, it depends on this patch to go in first. I'll send that out for review tomorrow.

Hi Amara,

Hi Sjoerd,

Changing the default to stable removes any concerns about zorg. I quite like this change as I do agree that all should be all. I also don't like the -W distinction, and this is where this change makes sense.

In D33410#790422, @DazedOwl wrote:

Yes using current HEAD (rev 306279) we are still getting the same failure, for both Linux and Windows.

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,