This is the first patch to incrementally add an MC layer for LoongArch to LLVM.
This patch also adds unit testcases for these new triples.
RFC for adding this new backend:
https://lists.llvm.org/pipermail/llvm-dev/2021-December/154371.html
Details
Details
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Comment Actions
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.
| llvm/lib/Support/Triple.cpp | ||
|---|---|---|
| 171 | 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. | |
| 1423 | I'm assuming this line has more than 80 columns... | |
| 1497 | same here | |
| llvm/unittests/ADT/TripleTest.cpp | ||
| 357 | 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. | |
| 1530 | more 80-columns? | |
Comment Actions
Address rengolin's comments.
| llvm/lib/Support/Triple.cpp | ||
|---|---|---|
| 171 | Seems that this is a common issue. case wasm32: case wasm64: return "wasm"; case riscv32: case riscv64: return "riscv"; | |
| 1423 | Actually this line only has 69 columns which is even shorter than line 1429. | |
| 1497 | same as above | |
| llvm/unittests/ADT/TripleTest.cpp | ||
| 357 | "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 I think we have 2 options:
| |
| 1530 | Yes. I will format this. Thanks. | |
| llvm/unittests/ADT/TripleTest.cpp | ||
|---|---|---|
| 357 | It's your target, you get to choose what the triples are. GCC probably wants to put "gnu" as the environment because that's what they provide, so they're not forced to conform to ill-specified external ABIs (not your case). If you have a specific ABI name and has issues with it being called "gnu" I think you can make the case to the GCC folks, but it usually isn't necessary unless it's a certification (or a marketing) issue. We don't really care what the targets are called, as long as they're used in the wild, which generally means GCC. So if you're happy with those triples, we're happy with them too. Regarding the two-step process, it doesn't matter. unknwon is a perfectly valid triple component, so feel free to leave it there, even after you add more triples. My comment was just for clarification. | |
Comment Actions
Address comments from @rengolin that add the CODE_OWNERS and CompilerWriterInfo parts (from 3/5) here.
Comment Actions
As previously discussed, we might want to wait until the gcc port is merged, to avoid code churn regarding triple names here.
Comment Actions
Those triples used in this patch are only for triple-parsing test and I think it doesn't affect triple definitions by gcc.
We can even add some dummy triple test like this, right?
T = Triple("loongarch64-unknown-darwin");
EXPECT_EQ(Triple::loongarch64, T.getArch());
EXPECT_EQ(Triple::UnknownVendor, T.getVendor());
EXPECT_EQ(Triple::Darwin, T.getOS());
EXPECT_EQ(Triple::UnknownEnvironment, T.getEnvironment());Comment Actions
Agreed. target.unknown.unknown should always work, regardless of what GCC does.
Those tests only check if the target is valid, completely ignoring the rest, which given what this patch introduces (just the target), it's a fair test.
Once GCC agrees on a set of triples, we can add them here again with a new patch.
Comment Actions
A gentle ping. Are there any futher comments? If no, could you accept the revision so that we can move on?
Comment Actions
Sorry, this fell out of my radar. Looks good to me, thanks for the hard work!
I'll review the rest of the patches and will ping specific people if there's any unresolved issues.
Comment Actions
Than you @SixWeining, the 6 patches have been merged now.
Be on the lookout for buildbot breakages (you'll get emails), but on a clean build on x86_64 + check-all I have seen no breakages.
Also thanks to @xen0n @myhsu for the reviews.
From now on, patches can come at any time, just make sure to announce the dependencies on each review, if they have one.
Feel free to continue to copy the people that helped you in the initial submission, and see if you can help other new and experimental targets as well (ARC, CSKY, M68k, MSP430, SPIR-V), with your gained experience in this process. The more you review other people's patches, the more likely they'll review yours.
Finally, welcome to the LLVM family!
Comment Actions
@rengolin @xen0n @myhsu Thanks for your help to make it happen. We will try our best to review other people's patches. We believe that goodwill is our currency.
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.