[X86] Support -mlong-double-80

[troyj]

Summary

Add an option group for all of the -mlong-double-* options and make -mlong-double-80 restore the default long double behavior for the targets that support the -mlong-double-64 and -mlong-double-128 options. The motivations are that GNU accepts the -mlong-double-80 option and that complex Makefiles often need a way of undoing earlier options. Prior to this commit, if one chooses 64-bit or 128-bit long double, there is no way to undo that choice and restore the 80-bit behavior.

[MaskRay]

clang/test/CodeGen/x86-long-double.cpp

19 ↗

(On Diff #214528)

We probably don't need more x86-long-double.cpp tests.

Instead, modify test/Driver/mlong-double-128.c:

-// RUN: %clang -target x86_64-linux-musl -c -### %s -mlong-double-128 2>&1 | FileCheck %s
+// RUN: %clang -target x86_64-linux-musl -c -### %s -mlong-double-128 -mlong-double-80 2>&1 | FileCheck --implicit-check-not=-mlong-double- /dev/null
+// RUN: %clang -target x86_64-linux-musl -c -### %s -mlong-double-80 -mlong-double-128 2>&1 | FileCheck --check-prefix=FP128 %s

CompilerInvocation.cpp has:

Opts.LongDoubleSize = Args.hasArg(OPT_mlong_double_128)
                          ? 128
                          : Args.hasArg(OPT_mlong_double_64) ? 64 : 0;

So it doesn't really check if a latter option overrides a former option.

[troyj]

OK, I moved it to a driver test. I agree that's more appropriate.

[MaskRay]

clang/test/Driver/mlong-double-128.c

5 ↗

(On Diff #214535)

You can delete this test now.

7 ↗

(On Diff #214535)

I think this comment is redundant. The intention is obvious by reading the test.

[troyj]

OK, I removed the other RUN line and the comment. I disagree with you about the comment, but I'm not going to hold up the patch for it -- I can keep it locally.

[MaskRay]

clang/lib/Driver/ToolChains/Clang.cpp

4050 ↗

(On Diff #214643)

powerpc gcc doesn't have -mlong-double-80.

You should also remove "[PowerPC]" from the title.

[hfinkel]

LGTM. As @MaskRay says, this shouldn't affect PowerPC, so you can remove the '[PowerPC]' from the title.

[troyj]

LGTM. As @MaskRay says, this shouldn't affect PowerPC, so you can remove the '[PowerPC]' from the title.

Oops, that's what I get for starting off copying the title of @MaskRay's previous reviews for the other options. :)

[troyj]

@MaskRay -- is this what you meant?

[MaskRay]

clang/test/Driver/mlong-double-128.c

12 ↗

(On Diff #214668)

powerpc-linux-musl -> powerpc (generic ELF platform). -mlong-double-80 is unavailable on any powerpc platform (not just linux).

[MaskRay]

Do you need me to change the test and commit for you? :)

[troyj]

Do you need me to change the test and commit for you? :)

I can change it. Got a bit delayed by work issuing me a new laptop.

I do not yet have commit access, though I plan on asking for it soon.

clang/lib/Driver/ToolChains/Clang.cpp

4050 ↗

(On Diff #214643)

So are you saying that this condition needs to be changed to reject the 80 option for TC.getArch() == llvm::Triple::ppc || TC.getTriple().isPPC64() ?

[troyj]

Changed target for test.

[MaskRay]

So are you saying that this condition needs to be changed to reject the 80 option for TC.getArch() == llvm::Triple::ppc || TC.getTriple().isPPC64() ?

% powerpc-linux-gnu-gcc -mlong-double-80 -fsyntax-only -xc /dev/null 
cc1: error: unknown switch -mlong-double-80
cc1: error: unrecognized command line option ‘-mlong-double-80’
% powerpc64le-linux-gnu-gcc -mlong-double-80 -fsyntax-only -xc /dev/null
cc1: error: unknown switch -mlong-double-80
cc1: error: unrecognized command line option ‘-mlong-double-80’

I believe -mlong-double-80 is also rejected on other powerpc targets (fp80 is a x86 specific thing), so we can use a generic ELF platform.

[troyj]

I believe -mlong-double-80 is also rejected on other powerpc targets (fp80 is a x86 specific thing), so we can use a generic ELF platform.

I'm not sure what you mean by "we can use a generic ELF platform." Are you suggesting that I change another condition? My area of expertise mainly covers x86 and that's really all that I care about for this option, so please let me know what needs to be changed for other targets.

[troyj]

@MaskRay - I have commit access now, so I'm just waiting on your approval that the patch is fine.

[MaskRay]

In D66055#1632525, @troyj wrote:

I believe -mlong-double-80 is also rejected on other powerpc targets (fp80 is a x86 specific thing), so we can use a generic ELF platform.

I'm not sure what you mean by "we can use a generic ELF platform." Are you suggesting that I change another condition? My area of expertise mainly covers x86 and that's really all that I care about for this option, so please let me know what needs to be changed for other targets.

If you write powerpc-linux-musl, people might think it can probably work on other OS or other environment. Use generic -target powerpc to make it clear (IMHO) it is not available on powerpc.

[leonardchan]

I believe this patch is causing 2 test failures on our x64 clang bots:

-- Testing: 15385 tests, 32 threads --
Testing: 0 .. 10.. 20.. 30
FAIL: Clang :: Driver/mlong-double-128.c (5227 of 15385)
******************** TEST 'Clang :: Driver/mlong-double-128.c' FAILED ********************
Script:
--
: 'RUN: at line 1';   /b/s/w/ir/k/recipe_cleanup/clangi0KG9E/llvm_build_dir/bin/clang -target powerpc-linux-musl -c -### /b/s/w/ir/k/llvm-project/clang/test/Driver/mlong-double-128.c -mlong-double-128 2>&1 | /b/s/w/ir/k/recipe_cleanup/clangi0KG9E/llvm_build_dir/bin/FileCheck /b/s/w/ir/k/llvm-project/clang/test/Driver/mlong-double-128.c
: 'RUN: at line 2';   /b/s/w/ir/k/recipe_cleanup/clangi0KG9E/llvm_build_dir/bin/clang -target powerpc64-pc-freebsd12 -c -### /b/s/w/ir/k/llvm-project/clang/test/Driver/mlong-double-128.c -mlong-double-128 2>&1 | /b/s/w/ir/k/recipe_cleanup/clangi0KG9E/llvm_build_dir/bin/FileCheck /b/s/w/ir/k/llvm-project/clang/test/Driver/mlong-double-128.c
: 'RUN: at line 3';   /b/s/w/ir/k/recipe_cleanup/clangi0KG9E/llvm_build_dir/bin/clang -target powerpc64le-linux-musl -c -### /b/s/w/ir/k/llvm-project/clang/test/Driver/mlong-double-128.c -mlong-double-128 2>&1 | /b/s/w/ir/k/recipe_cleanup/clangi0KG9E/llvm_build_dir/bin/FileCheck /b/s/w/ir/k/llvm-project/clang/test/Driver/mlong-double-128.c
: 'RUN: at line 4';   /b/s/w/ir/k/recipe_cleanup/clangi0KG9E/llvm_build_dir/bin/clang -target i686-linux-gnu -c -### /b/s/w/ir/k/llvm-project/clang/test/Driver/mlong-double-128.c -mlong-double-128 2>&1 | /b/s/w/ir/k/recipe_cleanup/clangi0KG9E/llvm_build_dir/bin/FileCheck /b/s/w/ir/k/llvm-project/clang/test/Driver/mlong-double-128.c
: 'RUN: at line 6';   /b/s/w/ir/k/recipe_cleanup/clangi0KG9E/llvm_build_dir/bin/clang -target x86_64-linux-musl -c -### /b/s/w/ir/k/llvm-project/clang/test/Driver/mlong-double-128.c -mlong-double-128 -mlong-double-80 2>&1 | /b/s/w/ir/k/recipe_cleanup/clangi0KG9E/llvm_build_dir/bin/FileCheck --implicit-check-not=-mlong-double- /dev/null
: 'RUN: at line 7';   /b/s/w/ir/k/recipe_cleanup/clangi0KG9E/llvm_build_dir/bin/clang -target x86_64-linux-musl -c -### /b/s/w/ir/k/llvm-project/clang/test/Driver/mlong-double-128.c -mlong-double-80 -mlong-double-128 2>&1 | /b/s/w/ir/k/recipe_cleanup/clangi0KG9E/llvm_build_dir/bin/FileCheck /b/s/w/ir/k/llvm-project/clang/test/Driver/mlong-double-128.c
: 'RUN: at line 11';   /b/s/w/ir/k/recipe_cleanup/clangi0KG9E/llvm_build_dir/bin/clang -target aarch64 -c -### /b/s/w/ir/k/llvm-project/clang/test/Driver/mlong-double-128.c -mlong-double-128 2>&1 | /b/s/w/ir/k/recipe_cleanup/clangi0KG9E/llvm_build_dir/bin/FileCheck --check-prefix=ERR /b/s/w/ir/k/llvm-project/clang/test/Driver/mlong-double-128.c
: 'RUN: at line 12';   /b/s/w/ir/k/recipe_cleanup/clangi0KG9E/llvm_build_dir/bin/clang -target powerpc -c -### /b/s/w/ir/k/llvm-project/clang/test/Driver/mlong-double-128.c -mlong-double-80 2>&1 | /b/s/w/ir/k/recipe_cleanup/clangi0KG9E/llvm_build_dir/bin/FileCheck --check-prefix=ERR2 /b/s/w/ir/k/llvm-project/clang/test/Driver/mlong-double-128.c
--
Exit Code: 1

Command Output (stderr):
--
/b/s/w/ir/k/llvm-project/clang/test/Driver/mlong-double-128.c:9:11: error: CHECK: expected string not found in input
// CHECK: "-mlong-double-128"
          ^
<stdin>:1:1: note: scanning from here
Fuchsia clang version 10.0.0 (https://fuchsia.googlesource.com/a/third_party/llvm-project 87869b398dbe525658d5ccfe2e9edd2cdf342662) (based on LLVM 10.0.0svn)
^
<stdin>:5:37: note: possible intended match here
clang-10: error: unsupported option '-mlong-double-128' for target 'powerpc64-pc-freebsd12'
                                    ^

--

********************
Testing: 0 .. 10.. 20.. 30
FAIL: Clang :: Driver/mlong-double-64.c (5230 of 15385)
******************** TEST 'Clang :: Driver/mlong-double-64.c' FAILED ********************
Script:
--
: 'RUN: at line 1';   /b/s/w/ir/k/recipe_cleanup/clangi0KG9E/llvm_build_dir/bin/clang -target powerpc-linux-musl -c -### /b/s/w/ir/k/llvm-project/clang/test/Driver/mlong-double-64.c -mlong-double-64 2>&1 | /b/s/w/ir/k/recipe_cleanup/clangi0KG9E/llvm_build_dir/bin/FileCheck /b/s/w/ir/k/llvm-project/clang/test/Driver/mlong-double-64.c
: 'RUN: at line 2';   /b/s/w/ir/k/recipe_cleanup/clangi0KG9E/llvm_build_dir/bin/clang -target powerpc64-pc-freebsd12 -c -### /b/s/w/ir/k/llvm-project/clang/test/Driver/mlong-double-64.c -mlong-double-64 2>&1 | /b/s/w/ir/k/recipe_cleanup/clangi0KG9E/llvm_build_dir/bin/FileCheck /b/s/w/ir/k/llvm-project/clang/test/Driver/mlong-double-64.c
: 'RUN: at line 3';   /b/s/w/ir/k/recipe_cleanup/clangi0KG9E/llvm_build_dir/bin/clang -target powerpc64le-linux-musl -c -### /b/s/w/ir/k/llvm-project/clang/test/Driver/mlong-double-64.c -mlong-double-64 2>&1 | /b/s/w/ir/k/recipe_cleanup/clangi0KG9E/llvm_build_dir/bin/FileCheck /b/s/w/ir/k/llvm-project/clang/test/Driver/mlong-double-64.c
: 'RUN: at line 4';   /b/s/w/ir/k/recipe_cleanup/clangi0KG9E/llvm_build_dir/bin/clang -target i686-linux-gnu -c -### /b/s/w/ir/k/llvm-project/clang/test/Driver/mlong-double-64.c -mlong-double-64 2>&1 | /b/s/w/ir/k/recipe_cleanup/clangi0KG9E/llvm_build_dir/bin/FileCheck /b/s/w/ir/k/llvm-project/clang/test/Driver/mlong-double-64.c
: 'RUN: at line 5';   /b/s/w/ir/k/recipe_cleanup/clangi0KG9E/llvm_build_dir/bin/clang -target x86_64-linux-musl -c -### /b/s/w/ir/k/llvm-project/clang/test/Driver/mlong-double-64.c -mlong-double-64 2>&1 | /b/s/w/ir/k/recipe_cleanup/clangi0KG9E/llvm_build_dir/bin/FileCheck /b/s/w/ir/k/llvm-project/clang/test/Driver/mlong-double-64.c
: 'RUN: at line 9';   /b/s/w/ir/k/recipe_cleanup/clangi0KG9E/llvm_build_dir/bin/clang -target aarch64 -c -### /b/s/w/ir/k/llvm-project/clang/test/Driver/mlong-double-64.c -mlong-double-64 2>&1 | /b/s/w/ir/k/recipe_cleanup/clangi0KG9E/llvm_build_dir/bin/FileCheck --check-prefix=ERR /b/s/w/ir/k/llvm-project/clang/test/Driver/mlong-double-64.c
--
Exit Code: 1

Command Output (stderr):
--
/b/s/w/ir/k/llvm-project/clang/test/Driver/mlong-double-64.c:7:11: error: CHECK: expected string not found in input
// CHECK: "-mlong-double-64"
          ^
<stdin>:1:1: note: scanning from here
Fuchsia clang version 10.0.0 (https://fuchsia.googlesource.com/a/third_party/llvm-project 87869b398dbe525658d5ccfe2e9edd2cdf342662) (based on LLVM 10.0.0svn)
^
<stdin>:5:37: note: possible intended match here
clang-10: error: unsupported option '-mlong-double-64' for target 'powerpc64-pc-freebsd12'
                                    ^

--

********************
Testing: 0 .. 10.. 20.. 30.. 40.. 50.. 60.. 70.. 80.. 90.. 
Testing Time: 78.20s
********************
Failing Tests (2):
    Clang :: Driver/mlong-double-128.c
    Clang :: Driver/mlong-double-64.c

Could you look into this? Thanks.

[troyj]

I believe this patch is causing 2 test failures on our x64 clang bots

On it...

[troyj]

I've reverted temporarily. I see the error in the mlong-double-128.c file, just by looking at the diff above (the target checked in the ERR2 line is wrong after I changed the RUN line above), but I don't understand why the mlong-double-64.c test broke.

cfe/trunk/include/clang/Driver/Options.td

def miamcu : Flag<["-"], "miamcu">, Group<m_Group>, Flags<[DriverOption, CoreOption]>,
   HelpText<"Use Intel MCU ABI">;
 def mno_iamcu : Flag<["-"], "mno-iamcu">, Group<m_Group>, Flags<[DriverOption, CoreOption]>;
 def malign_functions_EQ : Joined<["-"], "malign-functions=">, Group<clang_ignored_m_Group>;
 def malign_loops_EQ : Joined<["-"], "malign-loops=">, Group<clang_ignored_m_Group>;
 def malign_jumps_EQ : Joined<["-"], "malign-jumps=">, Group<clang_ignored_m_Group>;
 def mfancy_math_387 : Flag<["-"], "mfancy-math-387">, Group<clang_ignored_m_Group>;
 def mlong_calls : Flag<["-"], "mlong-calls">, Group<m_Group>,
   HelpText<"Generate branches with extended addressability, usually via indirect jumps.">;
-def mlong_double_64 : Flag<["-"], "mlong-double-64">, Group<f_Group>, Flags<[CC1Option]>,
+def LongDouble_Group : OptionGroup<"<LongDouble group>">, Group<m_Group>,
+  DocName<"Long double flags">,
+  DocBrief<[{Selects the long double implementation}]>;
+def mlong_double_64 : Flag<["-"], "mlong-double-64">, Group<LongDouble_Group>, Flags<[CC1Option]>,
   HelpText<"Force long double to be 64 bits">;
-def mlong_double_128 : Flag<["-"], "mlong-double-128">, Group<f_Group>, Flags<[CC1Option]>,
+def mlong_double_80 : Flag<["-"], "mlong-double-80">, Group<LongDouble_Group>, Flags<[CC1Option]>,
+  HelpText<"Force long double to be 80 bits, padded to 128 bits for storage">;
+def mlong_double_128 : Flag<["-"], "mlong-double-128">, Group<LongDouble_Group>, Flags<[CC1Option]>,
   HelpText<"Force long double to be 128 bits">;
 def mno_long_calls : Flag<["-"], "mno-long-calls">, Group<m_Group>,
   HelpText<"Restore the default behaviour of not generating long calls">;
 def mexecute_only : Flag<["-"], "mexecute-only">, Group<m_arm_Features_Group>,
   HelpText<"Disallow generation of data access to code sections (ARM only)">;
 def mno_execute_only : Flag<["-"], "mno-execute-only">, Group<m_arm_Features_Group>,
   HelpText<"Allow generation of data access to code sections (ARM only)">;
 def mtp_mode_EQ : Joined<["-"], "mtp=">, Group<m_arm_Features_Group>, Values<"soft,cp15,el0,el1,el2,el3">,

cfe/trunk/lib/Driver/ToolChains/Clang.cpp

Args.AddLastArg(CmdArgs, options::OPT_ffine_grained_bitfield_accesses,
                   options::OPT_fno_fine_grained_bitfield_accesses);
 
   // Handle segmented stacks.
   if (Args.hasArg(options::OPT_fsplit_stack))
     CmdArgs.push_back("-split-stacks");
 
   RenderFloatingPointOptions(TC, D, OFastEnabled, Args, CmdArgs);
 
-  if (Arg *A = Args.getLastArg(options::OPT_mlong_double_64,
-                               options::OPT_mlong_double_128)) {
+  if (Arg *A = Args.getLastArg(options::OPT_LongDouble_Group)) {
     if (TC.getArch() == llvm::Triple::x86 ||
-        TC.getArch() == llvm::Triple::x86_64 ||
-        TC.getArch() == llvm::Triple::ppc || TC.getTriple().isPPC64())
+        TC.getArch() == llvm::Triple::x86_64)
+      A->render(Args, CmdArgs);
+    else if ((TC.getArch() == llvm::Triple::ppc ||
+              TC.getArch() == TC.getTriple().isPPC64()) &&
+             (A->getOption().getID() != options::OPT_mlong_double_80))
       A->render(Args, CmdArgs);
     else
       D.Diag(diag::err_drv_unsupported_opt_for_target)
           << A->getAsString(Args) << TripleStr;
   }
 
   // Decide whether to use verbose asm. Verbose assembly is the default on
   // toolchains which have the integrated assembler on by default.

cfe/trunk/test/Driver/mlong-double-128.c

 // RUN: %clang -target powerpc-linux-musl -c -### %s -mlong-double-128 2>&1 | FileCheck %s
 // RUN: %clang -target powerpc64-pc-freebsd12 -c -### %s -mlong-double-128 2>&1 | FileCheck %s
 // RUN: %clang -target powerpc64le-linux-musl -c -### %s -mlong-double-128 2>&1 | FileCheck %s
 // RUN: %clang -target i686-linux-gnu -c -### %s -mlong-double-128 2>&1 | FileCheck %s
-// RUN: %clang -target x86_64-linux-musl -c -### %s -mlong-double-128 2>&1 | FileCheck %s
+
+// RUN: %clang -target x86_64-linux-musl -c -### %s -mlong-double-128 -mlong-double-80 2>&1 | FileCheck --implicit-check-not=-mlong-double- /dev/null
+// RUN: %clang -target x86_64-linux-musl -c -### %s -mlong-double-80 -mlong-double-128 2>&1 | FileCheck %s
 
 // CHECK: "-mlong-double-128"
 
 // RUN: %clang -target aarch64 -c -### %s -mlong-double-128 2>&1 | FileCheck --check-prefix=ERR %s
+// RUN: %clang -target powerpc -c -### %s -mlong-double-80 2>&1 | FileCheck --check-prefix=ERR2 %s
 
 // ERR: error: unsupported option '-mlong-double-128' for target 'aarch64'
+// ERR2: error: unsupported option '-mlong-double-80' for target 'powerpc-linux-musl'