Page MenuHomePhabricator

[LoongArch 1/6] Add triples loongarch{32,64} for the upcoming LoongArch target
ClosedPublic

Authored by SixWeining on Dec 16 2021, 12:41 AM.

Details

Summary

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

Diff Detail

Event Timeline

SixWeining created this revision.Dec 16 2021, 12:41 AM
SixWeining requested review of this revision.Dec 16 2021, 12:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 16 2021, 12:41 AM
SixWeining edited the summary of this revision. (Show Details)Dec 16 2021, 2:06 AM
SixWeining added a subscriber: chandlerc.

Add @chandlerc as reviewer since this change covers ADT and Support.

rename the patches number n to 5

SixWeining retitled this revision from [LoongArch 1/n] Add triples loongarch{32,64} for the upcoming LoongArch target to [LoongArch 1/5] Add triples loongarch{32,64} for the upcoming LoongArch target.Dec 17 2021, 7:55 PM

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?

Address rengolin's comments.

llvm/lib/Support/Triple.cpp
171

Seems that this is a common issue.
For example:
line 161-165

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
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.
  2. no change.
1530

Yes. I will format this. Thanks.

rengolin added inline comments.Dec 20 2021, 5:52 AM
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.

Adding owners of recent new targets to help onboarding this new target.

format the change in TripleTest.cpp

Address comments from @rengolin that add the CODE_OWNERS and CompilerWriterInfo parts (from 3/5) here.

These are all n/6 now, better to rename them all.

SixWeining retitled this revision from [LoongArch 1/5] Add triples loongarch{32,64} for the upcoming LoongArch target to [LoongArch 1/6] Add triples loongarch{32,64} for the upcoming LoongArch target.Dec 22 2021, 5:03 PM

These are all n/6 now, better to rename them all.

OK, I will name them all.

xen0n added a subscriber: xen0n.Jan 6 2022, 5:59 AM

As previously discussed, we might want to wait until the gcc port is merged, to avoid code churn regarding triple names here.

As previously discussed, we might want to wait until the gcc port is merged, to avoid code churn regarding triple names here.

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());

Those triples used in this patch are only for triple-parsing test and I think it doesn't affect triple definitions by gcc.

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.

A gentle ping. Are there any futher comments? If no, could you accept the revision so that we can move on?

rengolin accepted this revision.Feb 8 2022, 3:22 AM

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.

This revision is now accepted and ready to land.Feb 8 2022, 3:22 AM
xen0n accepted this revision.Feb 8 2022, 4:43 AM

For the record: the changes LGTM from a final (cursory) look.

This revision was landed with ongoing or failed builds.Feb 10 2022, 2:23 AM
This revision was automatically updated to reflect the committed changes.

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!

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!

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