[PATCH] D116879: [llvm] Allow auto-vectorization of sincos() using libmvec

[Renato Golin via Phabricator via llvm-commits]

rengolin added a comment.

In D116879#3232537 <https://reviews.llvm.org/D116879#3232537>, @tim.schmielau wrote:

> I have beefed up my testcase to demonstrate why I had to choose the `_ZGVdN4vvv_sincos()` variant for correctness, even though `_ZGVdN4vl8l8_sincos()` would be desirable from a performance perspective:
> We have no control over what pointers the user is passing in in different loop iterations.

That is true, but the vectoriser won't generate code that it deems unsafe (no known bounds, aliasing) and that's why I'm assuming you need the pragmas to force vectorisation in your tests.

In your example below, `size` is known and the compiler assumes access to the `[ith]` element from each pointer is sane (even though it could be undefined), and can vectorise the call inside the loop, regardless of what the original pointers had in hand.

> And a variant of the code above shows that even the transformation to the `vvv` variant isn't safe in all cases.

Is this with pragma or without? The compiler sometimes treats pragmas as "the user said it's safe, then it probably is".

Does this code generate unsafe transformations without any pragma or forced parameters?

> I am looking into adding variations of the code above into the test-suite ahead of enabling the vectorizing transformation, to be sure the transformation is not applied when unsafe, and that the behavior of the underlying vector library matches my interpretation of the VectorAPI.
> I don't see any existing tests around the transformations already performed.

Awesome, thanks!

@fpetrogalli @spatel @fhahn do you know of any tests for math library vectorisation?

> Also, I see lots of regression tests to prevent potential performance regressions, ensuring the vectorizing transformation is not missed. But I don't see any tests currently to guard against correctness issues, ensuring the transformation is not applied in unsafe cases.

It's harder to create adversarial tests than benign ones, unfortunately. That's not an excuse, just a fact. :)

It'd be awesome if we had more of such tests... (wink)

> I think there is groundwork to be done before this change can be made in confidence. So please do not yet commit, even if someone should approve.

Ack. We usually don't merge other people's patches unless they ask for it (like those that don't have commit permissions), so we should be safe.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116879