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

[Renato Golin via Phabricator via llvm-commits]

rengolin added a comment.

Other than the formatting hints, this looks ok to me. I'd prefer to add the CODE_OWNERS and CompilerWriterInfo parts (from 3/5) here, otherwise "loongarch" will make no sense if people try to search for it during a bisect.



================
Comment at: llvm/lib/Support/Triple.cpp:171
+  case loongarch32:
+  case loongarch64: return "loongarch";
   }
----------------
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.


================
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:
----------------
I'm assuming this line has more than 80 columns...


================
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:
----------------
same here


================
Comment at: llvm/unittests/ADT/TripleTest.cpp:357
 
+  T = Triple("loongarch32-unknown-unknown");
+  EXPECT_EQ(Triple::loongarch32, T.getArch());
----------------
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.


================
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());
----------------
more 80-columns?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115857