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

Fri Nov 23 23:52:57 PST 2018

chandlerc added a comment.

In D53927#1304262 <https://reviews.llvm.org/D53927#1304262>, @fpetrogalli wrote:

> @joel_k_jones , @steleman
>
> I am sorry for having kept you waiting for the patch. I was not fully aware of the approval process upstream (didn’t do much contributions upstream myself), and before being able to approve I had to understand the process.
>
> I will approve the patch and you will be able to submit it, but before doing that I would like to ask another tiny change.

FWIW, I'm sorry to push on this, but I think it would be better to have review from folks more familiar w/ generic LLVM optimizations and with SLEEF to review this. The obvious candidate, IMO, would be Hal. If Hal is unavailable, we could have a number of other people that would be very effective at reviewing (Tim, others...) with deep and londstanding experience in this place.

Given that this is a pretty significant change for AArch64 at least, and potentially for LLVM as a whole, so I think it is reasonable to expect a bit more wide review before it gets landed. Essentially, I'd suggest being a bit more reluctant to approve this kind of patch. =D

That said, I also want to say a huge thank you for actually reviewing (and doing a really detailed review!) of patches like this. It's really helpful, and please don't stop doing taht. It's simply amazing and definitely helps our process scale.

Repository:
  rL LLVM

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

https://reviews.llvm.org/D53927