[PATCH] D53927: [AArch64] Enable libm vectorized functions via SLEEF

Wed Feb 27 01:28:03 PST 2019

rengolin added a comment.

Hi @steleman, that wasn't a "loaded question", it was an honest one. Let me try to clarify what that means for all parties.

> Because:
> 
> It adds unnecessary complexity to this changeset and to D53928 <https://reviews.llvm.org/D53928>.
>  There is no obvious benefit to this added complexity.
>  There are many drawbacks to this added complexity.

Every split or merge adds complexity, but when submitting patches upstream, you are required to explain not only what you've done, but why you haven't done some other way. This is the dynamics of upstream review and is not meant to push back or demerit, but to make sure all assumptions are shared and there is consensus.

A better answer would be:

- because puling it apart would essentially create two very similar patch sets, possibly the second overwriting behaviour
- because we would need to make sure the intermediate state is sane (to avoid crashes in bisects) and that means testing a state that doesn't make sense
- because the final subset is too small, next to the refactoring needed, and the refactoring is part of "adding" the functionality

in addition to your own:

> Splitting it based on new vs. existing intrinsics seems arbitrary. One might as well ask for a separate changeset for each and every new intrinsic.

To answer your questions...

> And, now it's my turn to ask:
> 
> Why should it be split?
>  What is the benefit of splitting it?
>  What will be accomplished by splitting it?
>  Does integrating it as is prevent, or hinder something else?

Again, that was a standard question in upstream reviews, especially LLVM, as we have a more aggressive commit policy (more trunk breakages, more bisects, more reverts). For that to work, we must introduce the least amount of breakages, even in intermediate states, so they have to be small ans testable.

Furthermore, reverting the refactoring plus the actual change is more complicated than the actual change. If the breakage is in your usage of the refactoring, we can only revert that, leaving the refactoring, and being able to test the refactoring without the new behaviour and with, to find if the original assumption was broken, or if it was some silly mistake on the usage of it.

> Is increased complexity - with all its drawbacks and pitfalls - a goal?

Don't assume ill intent, those questions are always worth asking and rarely meant as anything else other than double (or triple) checking consensus.

> Why does this come up now, after this changeset - and its friend D53928 <https://reviews.llvm.org/D53928> - have been reviewed for 4+ months, and already approved, and re-approved, and there are 21 subscribers to this changeset?

Again, another standard upstream behaviour (on Linux, GCC and others as well).

People are copied in all sorts of reviews, code owners even more. If we were to review everything that people copy us in straight away we wouldn't be able to do anything else, including our day jobs.

Matt's and Eric's comments here are very welcome, and I would appreciate their review on our comments about the design decisions. If they agree, we have more hope that this was the right thing all along. If they have more questions, proposals and change requests, we will be relieved that they caught something we didn't before users started relying on it.

It does take some time, but when it goes in, there's a higher probability of being right.

Given that this change (and its Clang counterpart) will add external user-visible behaviour that will have to be maintained for a long time, it's only right that more people chime in when they have time.

I'm glad Matt and Eric took the time before it went in.

cheers,
--renato

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D53927/new/

https://reviews.llvm.org/D53927