Re: enabling float128 support in clang?

On Tue, Oct 04, 2022 at 04:02:05PM +0100, Alexander Richardson wrote:
> On Mon, 3 Oct 2022 at 19:43, Brooks Davis <brooks@freebsd.org> wrote:
> >
> > TL;DR: Is there a reason not to enable float128 support in clang for
> > x86?
> >
> > Due to a build breakage with the flang runtime, I recently noticed that
> > unlike most other OSes we don't support float128 in clang.  It's enable
> > in modern GCC and other OSes have it enable in clang:
> >  - Hakiu 2018: https://reviews.llvm.org/D54901
> >  - Solaris 2018: https://reviews.llvm.org/D41240
> >  - WASM 2019: https://reviews.llvm.org/D57154
> >  - Dragonfly 2021: https://reviews.llvm.org/D111760
> >  - OpenBSD (history hard to find...)
> >
> > Is there a known reason not to enable this?
> >
> > Patch to enable below (I'm a bit skeptical of the __FLOAT128__ part as
> > GCC doesn't define it...)
> >
> > -- Brooks
> >
> > diff --git a/clang/lib/Basic/Targets/OSTargets.h b/clang/lib/Basic/Targets/OSTargets.h
> > index c75f7d9fbafe..ea95f40e81a0 100644
> > --- a/clang/lib/Basic/Targets/OSTargets.h
> > +++ b/clang/lib/Basic/Targets/OSTargets.h
> > @@ -232,6 +232,9 @@ protected:
> >      // setting this to 1 is conforming even if all the basic source
> >      // character literals have the same encoding as char and wchar_t.
> >      Builder.defineMacro("__STDC_MB_MIGHT_NEQ_WC__", "1");
> > +
> > +    if (this->HasFloat128)
> > +      Builder.defineMacro("__FLOAT128__");
> >    }
> >
> >  public:
> > @@ -241,6 +244,7 @@ public:
> >      default:
> >      case llvm::Triple::x86:
> >      case llvm::Triple::x86_64:
> > +      this->HasFloat128 = true;
> >        this->MCountName = ".mcount";
> >        break;
> >      case llvm::Triple::mips:
> >
> 
> I think this makes sense and may even motivate me to try and get the
> necessary library calls merged to compiler-rt
> (https://reviews.llvm.org/D98261).

Sounds good. I'll submit a review.

> One thing to note is that your current patch will also enable float128
> for AArch64 (and other targets), which I don't believe any other OS
> does.
> I think it makes sense to restrict it to just x86 to avoid executing
> code in the compiler that has not been well tested. I believe for all
> other supported architectures long double is the same as float128 so
> it probably doesn't make too much sense to enable it there anyway.

Oops, that was an oversight.  Thanks for catching.  I'll restructure the
block like the OpenBSD one (x86 falls through to default).

-- Brooks