Do we need to handle integer vectors too?

Are these all places we need to handle these new intrinsics? Searching by minnum or maxnum in the llvm directory yields other occurrences in files like lib/Analysis/ValueTracking.cpp, lib/Analysis/VectorUtils.cpp, and lib/Analysis/InstructionSimplify.cpp. (There are more). For test cases as well.

Thank you!

Then maybe it's better to add test for ordered ones too..?

• Address comments

@sunfish Oh I see. Thank you!

1. Is it correct to translate those llvm instructions to saturating versions of wasm instructions? For example, for scalars, we had a patch that implemented the saturating behavior. Don't we need a similar thing for vectors too?
2. Is this behavior also dependent on nontrapping-fptoint feature, as the scalar instructions?

What happens if we use ordered comparisons? They fail to be selected?

1. What happens when only some of the elements in a vector are NaN?
2. What happens we have -0.0 in a vector?

I was not sure if what @dschuff asked was done. If it was I’m ok with the change.

Oh, what I meant is, the right operand is created not with insertelement + shufflevector but with just a constant vector whose all elements are the same integer. My comment was not very accurate because I think I remember you included that constant pattern as a splat too. So I guess it would work, but having a test case for that wouldn't hurt.

Does this work when the first argument is not in a splat pattern as well? If so, maybe add a test case for that?

Thanks! Could you please update diff for this CL then so it does not contain D52110?

Nit: Can we move the file name changing part to a separate CL?

I think this is dependent on D52007. Fixed it. By the way, the diff looks like it contains D52007's diff too. Could you fix it? You can do relative diff by
arc diff --update D52009 DIVISION_TO_COMPARE

@dschuff Re: about your question asked in person:
If a reg operand gets tied to another operand in instruction selection, they become the same register in here in TwoAddressInstructionPass. So they are assigned to a same local in this ExplicitLocal pass, so we don't need to worry two tied operands ending up in different locals.

Without this, this assertion fails.

• Add comment

I'm not actually sure if removing -wasm-disable-explicit-locals or -wasm-keep-registers from test cases is a direction we should head towards, because that generally makes test cases harder to read, as @aardappel D51459. Especially removing -wasm-keep-registers makes writing test cases very hard. Leaving these options as they are and assign 0 as a fake opcode to TEE_LOCAL might be an option too, as I suggested the other day. But that's just how I feel, and I'm not sure if we agreed on gradually removing those options from the test cases. What do you think, @dschuff and @aardappel ?

How did you resolve it in D51498 then?

Which aspect of the upcoming CL breaks it?

I'm checking. So according to the current shuffle spec, only half the number of elements from the two operands can be selected, right?

Rebase

Thank you everyone for comments! I'll land this then. :)

@sunfish Thanks. I agree that we have to respect the original file's style when we make changes to existing files, especially those that are not exclusive to wasm. But in these wasm-only files we didn't particularly have another style that deviates from LLVM, other than a few places of switch-cases. So I thought it might be possible to ensure clang-formattedness for wasm-exclusive code for now and we can recommend that for future committers as well.

Ping. Does anyone have more opinions on this? Do you think I can land this or this is not a good idea?

Ping pong

Ping! Can we land this because this is fairly small?

I'm totally OK with not landing this if there are any objections. :) But if we are ever gonna pursue clang-formattedness on wasm backend, I think doing it in a single CL that everyone is aware of makes it actually easier than having dozens of reformatting CLs when you trace history back using git blame or something. We can of course not do this, which is totally fine.

Also, among the hundreds of lines in this patch, it looks 80% of them are from switch-cases, when people prefer

switch SOMETHING: break;

over