[llvm-commits] [llvm-gcc-4.2] r55796 - /llvm-gcc-4.2/trunk/gcc/llvm-convert.cpp

[Dale Johannesen]

On Sep 5, 2008, at 12:03 PMPDT, Duncan Sands wrote:

> Hi,
>
>> This doesn't do the right thing for Darwin, and is inconsistent with
>> the existing handling of sqrt, which is explicitly tested in the
>> testsuite.
>
> what does it do wrong on Darwin?

Not generate the intrinsic.

> And it is not inconsistent with
> the handling of sqrt, because the gcc sqrt builtin should be
> readnone if and only if it doesn't set errno (ok, maybe it will
> be readonly if it doesn't set errno - depends on the platform).

Well, it is inconsistent,  look at the gcc code in builtins.def.  All  
the functions in
question have the same attribute, ATTR_MATHFN_FPROUNDING_ERRNO,
which is:

   errno_math ? neither const nor pure : unsafe_math ? const : pure

There is no reason for sqrt to be handled differently than the others,  
and the
test for sqrt (FrontendC/2005-07-20-SqrtNoErrno.c) is explictly passing
-fno-math-errno, not anything else.  Your patch would break this test.

>> The difficulty does seem to be the rounding mode.  I don't agree that
>> reading the rounding mode should prevent marking things "readnone";
>> the description can be read that way, but it's pretty clear whoever
>> wrote it wasn't thinking of rounding mode.
>
> I had rounding mode in mind when I added all this stuff.

OK.

> The gcc people set math functions as pure (readonly)
> rather than const (readnone) exactly because of the
> rounding mode, except for some platforms (Darwin)
> and with some options (fast-math).
>
>> Rounding mode is not memory.
>
> Suppose I call pow, then I change the rounding mode, then call
> pow again.  The result the second time may well be different.
> But if pow is readnone then GVN and other passes will think
> the result is the same and only call pow once.  This is just
> wrong.
>
>> There is no reason a libcall that reads rounding mode, but
>> doesn't touch memory, can't be moved across loads and stores.
>
> A readnone function can be moved across anything, including
> calls to the function setting rounding mode!

OK, that's true, but readonly is too restrictive, because you'll miss  
moving things
across stores.   "reads rounding mode only" needs to be represented
by something in between.

>> (It can't be moved across instructions that change the rounding  
>> mode, but
>> we don't currently model that, and this consideration is orthogonal  
>> to
>> touching memory.)
>
> The rounding mode can be considered a memory location, it's just
> that it's kept inside the processor rather than in main memory.

This would be correct, but is an overly conservative approach.

IMO worrying about the rounding mode when we don't model it at all is  
premature (I don't believe anyone has complained about the behavior of  
sqrt, which has all the problems you note above).  But I can see where  
some would prefer a conservatively correct implementation.  I guess  
what I can do is mark these intrinsics (including sqrt) as readonly  
and leave the tests in llvm-gcc alone.  Does that work for you?  (If  
it causes performance problems I may need to back that out, but I  
don't anticipate this.)