[PATCH] D136146: [Clang][LoongArch] Handle -march/-m{single,double,soft}-float/-mfpu options

[Lu Weining via Phabricator via cfe-commits]

SixWeining added a comment.

Sorry for the late reply.

Should we choose not to implement the `-mfpu=` option which is not mandatory?



================
Comment at: clang/lib/Driver/ToolChains/Arch/LoongArch.cpp:31
+                                     options::OPT_msingle_float,
+                                     options::OPT_msoft_float)) {
+    if (A->getOption().matches(options::OPT_mdouble_float))
----------------
MaskRay wrote:
> This is strange. Other architectures using -msoft-float makes it orthogonal to -mdouble-float...
> 
> Instead of having -mdouble-float/-msingle-float, can you just use -mfloat-abi=?
Sorry for the late reply.

Similar to @xry111's explanation below, `-m{double,single,soft}-float` are different from `-mfpu={64,32,0,none}`.

`-m{double,single,soft}-float` affect both codegen-ed instructions and the ABI while `-mfpu={64,32,0,none}` only affect the codegen-ed instructions.

That is to say:
* `-mdouble-float` implies `-mfpu=64` and `-mabi={lp64d,ilp32d}`;
* `-msingle-float` implies `-mfpu=32` and `-mabi={lp64f,ilp32f}`;
* `-msoft-float` implies `-mfpu={0,none}` and `-mabi={lp64s,ilp32s}`.

The `-mfpu={64,32,0,none}` clang option is like the `--mattr=+{d,f}` llc option. And the `-mabi=` clang option is like the `--target-abi=` llc option.

But I have to admit this introduce some complexity to clang implementation because we have to take care of the order these options appear on the command line.  The doc says:
> As a general rule, the effect of all LoongArch-specific compiler options that are given for one compiler invocation should be as if they are processed in the order they appear on the command line. The only exception to this rule is -m*-float: their configuration of floating-point instruction set and calling convention will not be changed by subsequent options other than -m*-float.



================
Comment at: clang/lib/Driver/ToolChains/Arch/LoongArch.cpp:44
 
+  // Select abi based on -mfpu=xx.
+  if (const Arg *A = Args.getLastArg(options::OPT_mfpu_EQ)) {
----------------
xry111 wrote:
> MaskRay wrote:
> > It's better to stick with just one canonical spelling. Is there time to remove support for one set of options? When `-mfpu=64 -msoft-float` is specified, is a -Wunused-command-line-argument warning expected?
> According to the doc, the semantics of -mfpu=64 and -mdouble-float are not exactly same.  `-mfpu=64 -mabi=lp64s` will allow the compiler to generate 64-bit floating-point instructions but keep LP64S calling convention.  `-mdouble-float -mabi=lp64s` will be same as `-mdouble-float -mabi=lp64d` (with a warning emitted, maybe).
The doc says that the implementation of `-mfpu` is not mandatory. So maybe we can choose not to implement it? But I'm not sure whether it has been used by any programs.

> -mdouble-float -mabi=lp64s will be same as -mdouble-float -mabi=lp64d (with a warning emitted, maybe).
Yes, I think so. GCC indeed emits a warning. We also should emit one.


================
Comment at: clang/test/Driver/loongarch-march-error.c:1
+// RUN: not %clang --target=loongarch64 -march=loongarch -fsyntax-only %s 2>&1 \
+// RUN:   | FileCheck --check-prefix=LOONGARCH %s
----------------
MaskRay wrote:
> The more common style is to place `|` in the end. The idea is that even without `\`, after typing the first line in a shell, it will wait for the second line.
OK. But seems both styles are used in the code base. -;)


================
Comment at: clang/test/Driver/loongarch-march.c:12
+// CC1-LOONGARCH64: "-target-feature" "+64bit"
+// CC1-LOONGARCH64-SAME: {{^}} "-target-feature" "+f"
+// CC1-LOONGARCH64-SAME: {{^}} "-target-feature" "+d"
----------------
MaskRay wrote:
> Check target-feature options on one line
OK.


================
Comment at: llvm/lib/Support/LoongArchTargetParser.cpp:46
+        if ((A.Features & F.Kind) == F.Kind && F.Kind != FK_INVALID) {
+          Features.push_back(F.Name);
+        }
----------------
MaskRay wrote:
> delete braces
OK.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136146