This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by SixWeining on Oct 18 2022, 2:17 AM.

Details

Summary

This patch adds options -march, -msingle-float, -mdouble-float,
-msoft-float and -mfpu for LoongArch.

Clang options msingle_float and mdouble_float are moved from
m_mips_Features_Group to m_Group because now more than one targets
use them.

Reference:
https://github.com/loongson/LoongArch-Documentation/blob/main/docs/LoongArch-toolchain-conventions-EN.adoc

TODO: add -mtune.

Diff Detail

Event Timeline

SixWeining created this revision.Oct 18 2022, 2:17 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 18 2022, 2:17 AM
SixWeining requested review of this revision.Oct 18 2022, 2:17 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptOct 18 2022, 2:17 AM
xen0n accepted this revision.Oct 30 2022, 7:29 PM

Looks good LoongArch-wise but I'm not very familiar with Clang internals to start with. Might be okay to land since this touches little code outside LoongArch and has passed tests.

This revision is now accepted and ready to land.Oct 30 2022, 7:29 PM
SixWeining edited the summary of this revision. (Show Details)Oct 31 2022, 7:10 AM
MaskRay added 1 blocking reviewer(s): MaskRay.Oct 31 2022, 11:47 AM
This revision now requires review to proceed.Oct 31 2022, 11:47 AM
MaskRay added inline comments.Nov 1 2022, 9:30 PM
clang/lib/Driver/ToolChains/Arch/LoongArch.cpp
31

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=?

44

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?

clang/test/Driver/loongarch-march-error.c
2

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.

clang/test/Driver/loongarch-march.c
13

Check target-feature options on one line

clang/test/Driver/loongarch-mdouble-float.c
9

Check target-feature options on one line

14

Replace {{[0-9]+}} with [[#]]

16

Delete unneeded comment

18

excess space

llvm/lib/Support/LoongArchTargetParser.cpp
36

delete braces

47

delete braces

MaskRay requested changes to this revision.Nov 1 2022, 9:30 PM
This revision now requires changes to proceed.Nov 1 2022, 9:30 PM
xry111 added inline comments.Nov 1 2022, 9:40 PM
clang/lib/Driver/ToolChains/Arch/LoongArch.cpp
44

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).

Sorry for the late reply.

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

clang/lib/Driver/ToolChains/Arch/LoongArch.cpp
31

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.

44

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.

clang/test/Driver/loongarch-march-error.c
2

OK. But seems both styles are used in the code base. -;)

clang/test/Driver/loongarch-march.c
13

OK.

llvm/lib/Support/LoongArchTargetParser.cpp
47

OK.

SixWeining updated this revision to Diff 473627.Nov 7 2022, 5:18 AM

Add test for warning: argument unused during compilation. And address other comments from @MaskRay.

MaskRay accepted this revision.Nov 9 2022, 9:34 AM

Sorry for the late reply.

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

OK, I accept the -mfpu= different from -m*-float. If the patch does not implement the required semantics, deferring -mfpu= implementation may be good.
We can also check how many pieces of software actually use -mfpu= and some unneeded uses may be removed?

clang/include/clang/Basic/DiagnosticDriverKinds.td
696

0, none (alias for 0) or none, 0 (alias for none)

Ideally only one spelling is retained... No need to implement an alias if -mfpu= itself is niche.

llvm/lib/Support/LoongArchTargetParser.cpp
47

for (const auto A : AllArchs) needs braces since the nested if (A.Name == Arch) { has braces.

This revision is now accepted and ready to land.Nov 9 2022, 9:34 AM
SixWeining updated this revision to Diff 474414.Nov 9 2022, 7:11 PM

Revise the err_drv_loongarch_invalid_mfpu_EQ diagnostic info. Add necessary braces.

This revision was landed with ongoing or failed builds.Nov 10 2022, 1:31 AM
This revision was automatically updated to reflect the committed changes.

This change was pushed yesterday, but it's not the last diff. I will submit a follow-up change as soon as possible.