[PATCH] D98574: [Sparc] Define the same macros for -mcpu=v9 as GCC on Linux and the BSDs

[Harald van Dijk via Phabricator via cfe-commits]

hvdijk added inline comments.


================
Comment at: clang/lib/Basic/Targets/Sparc.cpp:246-256
+  if (getTriple().getOS() == llvm::Triple::Linux) {
     Builder.defineMacro("__sparc_v9__");
-    Builder.defineMacro("__sparcv9__");
+  } else {
+    Builder.defineMacro("__sparcv9");
+    // Solaris doesn't need these variants, but the BSDs do.
+    if (getTriple().getOS() != llvm::Triple::Solaris) {
+      Builder.defineMacro("__sparc64__");
----------------
brad wrote:
> glaubitz wrote:
> > glaubitz wrote:
> > > ro wrote:
> > > > glaubitz wrote:
> > > > > jrtc27 wrote:
> > > > > > This doesn't need changing, we can define more things than GCC to keep it simple.
> > > > > Well, my original intent was to match GCC to make sure we're 100% compatible and I would like to keep it that way.
> > > > I agree with Jessica here: you're creating a complicated maze for no real gain.  Besides, have you checked what `gcc` on the BSDs really does?  They often neglect to get their changes upstream and what's in the gcc repo doesn't necessarily represent what they actually use.
> > > Yes, I have verified that GCC behaves the exact same way as this change and I don't see any reason not to mimic the exact same behavior in clang for maximum compatibility.
> > FWIW, I meant GCC on the various BSDs. I do not think it's a wise idea to have clang deviate from what GCC does as only this way we can guarantee that everything that compiles with GCC will compile with clang.
> > Besides, have you checked what `gcc` on the BSDs really does?  They often neglect to get their changes upstream and what's in the gcc repo doesn't necessarily represent what they actually use.
> 
> What is upstream is what we do. There are no local patches that change behavior in this particular area.
(Copying here what I had already replied privately a while back) It worries me that this switch statement only handles known operating systems (Linux, FreeBSD, NetBSD, OpenBSD, Solaris) when we also have code to allow SparcV9TargetInfo to be created without an operating system in clang/lib/Basic/Targets.cpp. Either there should be a default case that is properly handled, or if that actually cannot happen, there should be an assert that it doesn't happen.

I agree with the earlier comments that there should be nothing wrong with defining more macros than GCC, if the macros make sense. For the SparcV9TargetInfo class, my impression is that the macros make sense. For the SparcV8TargetInfo class with a V9 CPU, reading the discussion in D86621, if Oracle say that `__sparcv9` is only for 64-bit mode, GCC also only defines it for 64-bit mode, glibc assumes that `__sparcv9` implies 64-bit mode, etc. then SparcV8TargetInfo should not be defining `__sparcv9`. Your changes to SparcV8TargetInfo should be enough to fix bug 49562, right? If so, would it be okay to update this diff with just that?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98574