[PATCH] D115857: [LoongArch 1/5] Add triples loongarch{32, 64} for the upcoming LoongArch target

[Lu Weining via Phabricator via llvm-commits]

SixWeining added a comment.

Address rengolin's comments.



================
Comment at: llvm/lib/Support/Triple.cpp:171
+  case loongarch32:
+  case loongarch64: return "loongarch";
   }
----------------
rengolin wrote:
> clang-format is probably complaining here because it makes it clearer that the return is for the two cases above when you break into a new line. This is a reasonable change.
Seems that this is a common issue.
For example:
line 161-165
```
  case wasm32:
  case wasm64:      return "wasm";

  case riscv32:
  case riscv64:     return "riscv";
```



================
Comment at: llvm/lib/Support/Triple.cpp:1419
   case Triple::le64:           T.setArch(Triple::le32);    break;
+  case Triple::loongarch64:    T.setArch(Triple::loongarch32); break;
   case Triple::mips64:
----------------
rengolin wrote:
> I'm assuming this line has more than 80 columns...
Actually this line only has 69 columns which is even shorter than line 1429.


================
Comment at: llvm/lib/Support/Triple.cpp:1493
   case Triple::le32:            T.setArch(Triple::le64);       break;
+  case Triple::loongarch32:     T.setArch(Triple::loongarch64);    break;
   case Triple::mips:
----------------
rengolin wrote:
> same here
same as above


================
Comment at: llvm/unittests/ADT/TripleTest.cpp:357
 
+  T = Triple("loongarch32-unknown-unknown");
+  EXPECT_EQ(Triple::loongarch32, T.getArch());
----------------
rengolin wrote:
> I'm assuming this is bare metal?
> 
> If you haven't fixed this elsewhere, you may want to do it now, for example, using the name of your ABI like Arm does (aeabi). 
> 
> But this is totally optional, because it doesn't make much of a difference if you only ever have one ABI for bare metal.
"loongarch32-unknown-unknown" is only a basic triple test for the loongarch32 target and here we just follow what other architectures do, like csky (line 345), riscv(line 369).

Currently GCC is proposing using below triples
loongarch64-linux-gnuf64
loongarch64-linux-gnuf32
loongarch64-linux-gnusf
loongarch32-linux-gnuf64
loongarch32-linux-gnuf32
loongarch32-linux-gnusf

I think we have 2 options:
1. Delete this triple test for loongarch32 and add it/them back until the environment names are upstreamed to GCC.
1. no change.


================
Comment at: llvm/unittests/ADT/TripleTest.cpp:1530
 
+  EXPECT_EQ(Triple::ELF, Triple("loongarch32-unknown-unknown").getObjectFormat());
+  EXPECT_EQ(Triple::ELF, Triple("loongarch64-unknown-linux").getObjectFormat());
----------------
rengolin wrote:
> more 80-columns?
Yes. I will format this. Thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115857