[PATCH] Handle sqrt() shrinking in SimplifyLibCalls like any other call

[Hal Finkel]

----- Original Message -----
> From: "Hal Finkel" <hfinkel at anl.gov>
> To: "Stephen Canon" <scanon at apple.com>
> Cc: llvm-commits at cs.uiuc.edu, spatel at rotateright.com, beanz at apple.com,
> reviews+D5919+public+763adead774985f1 at reviews.llvm.org
> Sent: Thursday, October 23, 2014 2:51:34 PM
> Subject: Re: [PATCH] Handle sqrt() shrinking in SimplifyLibCalls like any other call
> 
> ----- Original Message -----
> > From: "Stephen Canon" <scanon at apple.com>
> > To: "Hal Finkel" <hfinkel at anl.gov>
> > Cc: llvm-commits at cs.uiuc.edu, spatel at rotateright.com,
> > beanz at apple.com,
> > reviews+D5919+public+763adead774985f1 at reviews.llvm.org
> > Sent: Thursday, October 23, 2014 2:46:17 PM
> > Subject: Re: [PATCH] Handle sqrt() shrinking in SimplifyLibCalls
> > like any other call
> > 
> > (float)sqrt((double)x) —> sqrtf(x) is a valid transformation [See
> > Sam
> > Figueroa’s thesis for details].
> 
> Does this assume that the sqrtf is correct to <= 0.5ulps?

I suppose that C99 F.3 does say that, "The sqrt functions in <math.h> provide the IEC 60559 square root operation.", so perhaps this is a safe assumption on a compliant system.

Alright, Sanjay, go ahead. You're not actually changing anything. ;)

 -Hal

> 
>  -Hal
> 
> > 
> > However, sqrt((double)x) —> (double)sqrtf(x) is not a valid
> > transformation [Use almost any non-perfect-square x for an
> > example].
> > 
> > – Steve
> > 
> > 
> > > On Oct 23, 2014, at 3:35 PM, Hal Finkel <hfinkel at anl.gov> wrote:
> > > 
> > > Hrmm... I'm not entirely comfortable with this. Steve, do you
> > > have
> > > an opinion?
> > > 
> > > -Hal
> > > 
> > > ----- Original Message -----
> > >> From: "Sanjay Patel" <spatel at rotateright.com>
> > >> To: spatel at rotateright.com, beanz at apple.com, hfinkel at anl.gov
> > >> Cc: llvm-commits at cs.uiuc.edu
> > >> Sent: Thursday, October 23, 2014 2:24:50 PM
> > >> Subject: Re: [PATCH] Handle sqrt() shrinking in SimplifyLibCalls
> > >> like any other call
> > >> 
> > >>>> ! In D5919#4, @hfinkel wrote:
> > >>> Please don't loosen this for the function call (although leave
> > >>> it
> > >>> as-is for the intrinsic). The intrinsic is different here,
> > >>> especially because it is defined to have different semantics
> > >>> than
> > >>> the function call, but we should not change the function call
> > >>> itself without fast-math enabled. The underlying issue is that
> > >>> (float) sqrt(x) != sqrtf(x) in general because of rounding
> > >>> issues
> > >>> (and on some systems, the sqrtf is not exactly 1ulp accurate),
> > >>> and
> > >>> we should not alter that without fast-math enabled.
> > >> 
> > >> Let me make sure we're on the same page. The existing behavior
> > >> is
> > >> to
> > >> transform this function call:
> > >>   float x;
> > >>   float y = (float) sqrt ((double) x)
> > >> 
> > >> or in IR:
> > >>   %conv = fpext float %f to double
> > >>   %call = call double @sqrt(double %conv)
> > >>   %conv1 = fptrunc double %call to float
> > >>   ret float %conv1
> > >> 
> > >> into:
> > >>  float y = sqrtf (x);
> > >> 
> > >> There's are 2 existing positive test cases in
> > >> test/Transform/InstCombine/sqrt.ll that check for this pattern
> > >> and
> > >> 1
> > >> negative test case to make sure we don't optimize in the event
> > >> the
> > >> result is used as a double.
> > >> 
> > >> Are you saying those existing test cases are invalid? I think
> > >> that
> > >> the sqrt function call test that I'm proposing to modify in
> > >> double-float-shrink-1.ll would actually become redundant with
> > >> the
> > >> second test case in sqrt.ll; that was added for:
> > >> http://llvm.org/bugs/show_bug.cgi?id=8096
> > >> 
> > >> http://reviews.llvm.org/D5919
> > >> 
> > >> 
> > >> 
> > > 
> > > --
> > > Hal Finkel
> > > Assistant Computational Scientist
> > > Leadership Computing Facility
> > > Argonne National Laboratory
> > 
> > 
> 
> --
> Hal Finkel
> Assistant Computational Scientist
> Leadership Computing Facility
> Argonne National Laboratory
> 

-- 
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory