- User Since
- Jul 29 2016, 12:33 AM (112 w, 4 d)
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.
Then maybe it's better to add test for ordered ones too..?
- Address comments
@sunfish Oh I see. Thank you!
- 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?
- 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?
- What happens when only some of the elements in a vector are NaN?
- What happens we have -0.0 in a vector?
Thu, Sep 20
I was not sure if what @dschuff asked was done. If it was I’m ok with the change.
Wed, Sep 19
Tue, Sep 18
Fri, Sep 14
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?
Thu, Sep 13
Nit: Can we move the file name changing part to a separate CL?
Wed, Sep 12
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
Tue, Sep 11
Mon, Sep 10
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?
Fri, Sep 7
I'm checking. So according to the current shuffle spec, only half the number of elements from the two operands can be selected, right?
Thu, Sep 6
Tue, Sep 4
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?
Fri, Aug 31
Thu, Aug 30
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.
Wed, Aug 29
Also, among the hundreds of lines in this patch, it looks 80% of them are from switch-cases, when people prefer
switch SOMETHING: break;