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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


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


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


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


================
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"
----------------
Check target-feature options on one line


================
Comment at: clang/test/Driver/loongarch-mdouble-float.c:8
+// CC1: "-target-feature" "+64bit"
+// CC1-SAME: {{^}} "-target-feature" "+f"
+// CC1-SAME: {{^}} "-target-feature" "+d"
----------------
Check target-feature options on one line


================
Comment at: clang/test/Driver/loongarch-mdouble-float.c:13
+
+// IR: attributes #{{[0-9]+}} ={{.*}}"target-features"="+64bit,+d,+f"
+
----------------
Replace `{{[0-9]+}}` with `[[#]]`


================
Comment at: clang/test/Driver/loongarch-mdouble-float.c:15
+
+/// Dummy function
+int foo(void) {
----------------
Delete unneeded comment


================
Comment at: clang/test/Driver/loongarch-mdouble-float.c:17
+int foo(void) {
+  return  3;
+}
----------------
excess space


================
Comment at: llvm/lib/Support/LoongArchTargetParser.cpp:35
+      return A.Kind;
+  }
+
----------------
delete braces


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136146