[PATCH] D110900: Triple: Add RedHat vendor

[Aaron Puchert via Phabricator via llvm-commits]

aaronpuchert added a comment.
Herald added a project: All.

In D110900#3044159 <https://reviews.llvm.org/D110900#3044159>, @aaronpuchert wrote:

> Can't speak for RedHat, but on SUSE we have
>
>   > gcc -dumpmachine
>   x86_64-suse-linux
>   > clang -dumpmachine
>   x86_64-unknown-linux-gnu

Not anymore. I've switched the default target triple to `<arch>-suse-linux` some time ago (except for ARM 6 & 7, where we specify the ABI), and haven't gotten any complaints about it.

There is only one issue that I have observed, and patching `isGNUEnvironment` fixes it: we get a function call for `fma` even if it's available in hardware, apparently because `Sema::AddKnownFunctionAttributes` in SemaDecl.cpp doesn't add `__attribute__(const))` if `isGNUEnvironment` returns false.



================
Comment at: llvm/include/llvm/ADT/Triple.h:532
   bool isGNUEnvironment() const {
     EnvironmentType Env = getEnvironment();
     return Env == Triple::GNU || Env == Triple::GNUABIN32 ||
----------------
aaronpuchert wrote:
> Since `getEnvironment` is also public, shouldn't we fix it there? Otherwise I fear this might lead to inconsistencies. (That is, `getEnvironment()` returning `UnknownEnvironment`, but `isGNUEnvironment()` returning `true`.) Perhaps we should even do something like this:
> 
> ```
>   bool isGNUEnvironment() const {
>     return isGNU(getEnvironment());
>   }
>   static bool isGNU(EnvironmentType Env) {
>     // ...
>   }
> ```
> 
> to clarify that `isGNUEnvironment` really only depends on the environment.
> 
> Possibly we should even set `Environment = Triple::GNU;` in the constructor for vendors known to use GNU by default without declaring it?
> 
> That's currently done in some cases, though I'm not sure if the members should contain the properties as declared, or the actual values.
I've been thinking about this a bit more and think what you did here is actually correct. The triple doesn't explicitly specify GNU, but we're "implicitly" in a GNU environment. And if one wants to check whether they're in some kind of GNU environment, they'd probably use this function instead of enumerating all the GNU* environments anyway. Most of the occurrences of `Triple::GNU` are actually in connection with `Triple::Win32`. Places where there might be issues:

* `ARMTargetInfo::ARMTargetInfo` has a `switch (Triple.getEnvironment())`. But I'm not familiar with this and haven't observed any issues.
* `computeTargetTriple` in `clang/lib/Driver/Driver.cpp` could be an issue with `-m32` or `-m64`.


================
Comment at: llvm/include/llvm/ADT/Triple.h:532-536
     EnvironmentType Env = getEnvironment();
     return Env == Triple::GNU || Env == Triple::GNUABIN32 ||
            Env == Triple::GNUABI64 || Env == Triple::GNUEABI ||
-           Env == Triple::GNUEABIHF || Env == Triple::GNUX32;
+           Env == Triple::GNUEABIHF || Env == Triple::GNUX32 ||
+           (getVendor() == Triple::RedHat && Env == Triple::UnknownEnvironment);
----------------
aaronpuchert wrote:
> aaronpuchert wrote:
> > Since `getEnvironment` is also public, shouldn't we fix it there? Otherwise I fear this might lead to inconsistencies. (That is, `getEnvironment()` returning `UnknownEnvironment`, but `isGNUEnvironment()` returning `true`.) Perhaps we should even do something like this:
> > 
> > ```
> >   bool isGNUEnvironment() const {
> >     return isGNU(getEnvironment());
> >   }
> >   static bool isGNU(EnvironmentType Env) {
> >     // ...
> >   }
> > ```
> > 
> > to clarify that `isGNUEnvironment` really only depends on the environment.
> > 
> > Possibly we should even set `Environment = Triple::GNU;` in the constructor for vendors known to use GNU by default without declaring it?
> > 
> > That's currently done in some cases, though I'm not sure if the members should contain the properties as declared, or the actual values.
> I've been thinking about this a bit more and think what you did here is actually correct. The triple doesn't explicitly specify GNU, but we're "implicitly" in a GNU environment. And if one wants to check whether they're in some kind of GNU environment, they'd probably use this function instead of enumerating all the GNU* environments anyway. Most of the occurrences of `Triple::GNU` are actually in connection with `Triple::Win32`. Places where there might be issues:
> 
> * `ARMTargetInfo::ARMTargetInfo` has a `switch (Triple.getEnvironment())`. But I'm not familiar with this and haven't observed any issues.
> * `computeTargetTriple` in `clang/lib/Driver/Driver.cpp` could be an issue with `-m32` or `-m64`.
Perhaps we should make this a switch?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D110900/new/

https://reviews.llvm.org/D110900