llvm dev - Jun 2016 - Sincos for X86_64's GNUX32 and ARM's GNUEABI/GNUEABIHF enviroments

Hi,

While writing http://reviews.llvm.org/D20916, I stumbled across some code
affecting ARM and X86_64 environments that looks like it might be unintentional.
I thought I should ask about it here since that patch has a '[mips]' tag
and therefore might not be noticed by someone who knows.

I've noticed that the GNUX32 and GNUEABI/GNUEABIHF environments don't
make use of the sincos libcall like the GNU environment does because the
guarding condition for this is 'TT.getEnvironment() == Triple::GNU'. The
comment in canCombineSinCosLibcall() in LegalizeDAG.cpp suggests that it's
intending to use sincos for all GNU environments so I'm wondering whether
the exclusion of GNUX32/GNUEABI/GNUEABIHF is deliberate or not.

Can anyone confirm?

Thanks.

On 15 June 2016 at 11:24, Daniel Sanders via llvm-dev
<llvm-dev at lists.llvm.org> wrote:
> I've noticed that the GNUX32 and GNUEABI/GNUEABIHF environments
don't make use of the sincos libcall like the GNU environment does because
the guarding condition for this is 'TT.getEnvironment() == Triple::GNU'.
The comment in canCombineSinCosLibcall() in LegalizeDAG.cpp suggests that
it's intending to use sincos for all GNU environments so I'm wondering
whether the exclusion of GNUX32/GNUEABI/GNUEABIHF is deliberate or not.
Hi Daniel,

I don't remember anything deliberate about that. But I haven't been
playing around sin/cos much. To me, it looks like an omission more
than anything.

cheers,
--renato

> -----Original Message-----
> From: Renato Golin [mailto:renato.golin at linaro.org]
> Sent: 16 June 2016 09:44
> To: Daniel Sanders
> Cc: llvm-dev at lists.llvm.org
> Subject: Re: [llvm-dev] Sincos for X86_64's GNUX32 and ARM's
> GNUEABI/GNUEABIHF enviroments
> 
> On 15 June 2016 at 11:24, Daniel Sanders via llvm-dev
> <llvm-dev at lists.llvm.org> wrote:
> > I've noticed that the GNUX32 and GNUEABI/GNUEABIHF environments
> don't make use of the sincos libcall like the GNU environment does
because
> the guarding condition for this is 'TT.getEnvironment() ==
Triple::GNU'. The
> comment in canCombineSinCosLibcall() in LegalizeDAG.cpp suggests that
it's
> intending to use sincos for all GNU environments so I'm wondering
whether
> the exclusion of GNUX32/GNUEABI/GNUEABIHF is deliberate or not.
> 
> Hi Daniel,
> 
> I don't remember anything deliberate about that. But I haven't been
> playing around sin/cos much. To me, it looks like an omission more
> than anything.
> 
> cheers,
> --renato
Thanks. Seeing as you think it's an omission as well, I've posted a
patch at http://reviews.llvm.org/D21431.

I've also realised I had the meaning of canCombineSinCosLibcall() backwards.
GNUX32/GNUEABI/GNUEABIHF are combining sin+cos into sincos in a situation where
GNU considers it to be unsafe.