[PATCH] D41599: [X86] Lowering X86 avx512 sqrt intrinsics to IR - LLVM

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

In https://reviews.llvm.org/D41599#976338, @RKSimon wrote:

> In https://reviews.llvm.org/D41599#976062, @uriel.k wrote:
>
> > Simon, is there anything else you think that is needed to be changed before accepting the revision?
> > Thanks
>
>
> I'm still a little worried about this - it can create a lot more bit differences in results than previous other intrinsics where we've replaced with generic implementations - I guess _mm_div_ps already does this to an extent (and other fadd/fsub/fmul cases via re-association etc.).
>
> Maybe I'm just being a little over cautious, but at very least I'd like to see https://reviews.llvm.org/D41168 update the intrinsic documentation to explain that -ffast-math may result in rsqrt+nr codgen under some circumstances - it still says that (v)sqrtps will be generated.


We want to allow more transforms with -ffast-math regardless of whether the source used intrinsics, so we shouldn’t treat sqrt differently than other math ops.

That said, updating the clang header docs is necessary too. We’re almost certainly going to see fallout from this change and get bug reports.
One nit: these patches are titled ‘avx512’ when they apply to all avx/sse. When committing, I recommend splitting these patches up by intrinsic (if not individually, then at least by sse/avx/avx512). This will reduce the risk that the whole thing gets reverted…that will also make it easier to pinpoint exactly which intrinsic is under investigation when the complaints come in.


https://reviews.llvm.org/D41599