This is an archive of the discontinued LLVM Phabricator instance.

[Clang][CSKY] Add the CSKY target and compiler driver
ClosedPublic

Authored by zixuan-wu on Mar 10 2022, 11:29 PM.

Details

Summary

Add CSKY target toolchains to support csky in linux and elf environment.

It can leverage the basic universal Linux toolchain for linux environment, and only add some compile or link parameters. For elf environment, add a CSKYToolChain to support compile and link.

Also add some parameters into basic codebase of clang driver.

Diff Detail

Event Timeline

zixuan-wu created this revision.Mar 10 2022, 11:29 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 10 2022, 11:29 PM
zixuan-wu requested review of this revision.Mar 10 2022, 11:29 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 10 2022, 11:29 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

I don't know enough about your toolchain requirements, but this looks good to me.

Please check the clang-format warnings. If you did pass clang-format, perhaps you need to upgrade to a newer one?

I won't approve just yet, to let other people review it also.

clang/test/Driver/csky-arch.c
27

I don't think this is doing what you expect it to do.

Depending on the output, you can still match all CHECK lines and not have the output you want.

To avoid issues, we usually separate tests that must pass (positive tests) like the lines above 24, from tests that must fail (negative tests) like the lines below.

DavidSpickett added inline comments.Mar 11 2022, 2:58 AM
clang/lib/Basic/Targets/CSKY.cpp
44

Any need to handle when ABI is not avbiv2 or abiv1 or will it always be one of the two?

clang/lib/Driver/ToolChains/Linux.cpp
380

Can you comment what all the .. are going to/from. I assume from some deep folder back up, down into the sysroot but an example in a comment would be good to document it.

zixuan-wu added inline comments.Mar 13 2022, 7:37 PM
clang/lib/Basic/Targets/CSKY.cpp
44

Yes. It always be one of the two.

zixuan-wu added inline comments.Mar 13 2022, 8:07 PM
clang/test/Driver/csky-arch.c
27

You mean separate into two files or two parts in one file?

Address comments.

Herald added a project: Restricted Project. · View Herald TranscriptMar 13 2022, 10:45 PM

I have updated the patch. Any more comments?

rengolin added inline comments.Mar 17 2022, 3:51 AM
clang/test/Driver/csky-arch-error.c
2

This will error out and fail the test. You need to add a not before %clang.

Check tests in clang/test/Driver/ that has not %clang as their RUN lines and do the same.

And keep all the not %clang tests separate from the %clang tests.

clang/test/Driver/csky-arch.c
29

If the RUN line fails (ex. on invalid arch name), the return value will be non-zero and the test will fail, so CHECKing for the absence of errors is no-op.

clang/test/Driver/csky-cpus.c
13

Same here, separate not tests.

I'm surprised these tests are passing for you. Perhaps you're not building or running them all.

To make sure you're running your tests, you need to build both clang and llvm (-DLLVM_ENABLE_PROJECTS=clang) and run ninja/make check-all.

You can also run lit directly on each test, but I can't remember how to do that now...

You can also run lit directly on each test, but I can't remember how to do that now...

If you're in the build dir:

./bin/llvm-lit ../llvm-project/clang/test/...... -a

(-a gives you full output of what happens)

Some tests are a bit odd when you do this but these clang tests are usually ok.

I'm surprised these tests are passing for you. Perhaps you're not building or running them all.

To make sure you're running your tests, you need to build both clang and llvm (-DLLVM_ENABLE_PROJECTS=clang) and run ninja/make check-all.

You can also run lit directly on each test, but I can't remember how to do that now...

I have enabled clang project building and run check-all test. Like -DLLVM_ENABLE_ASSERTIONS=On -DLLVM_ENABLE_PROJECTS="clang;llvm" -DLLVM_EXPERIMENTAL_TARGETS_TO_BUILD="CSKY". So I am also surprised why those cases passed.

zixuan-wu updated this revision to Diff 419076.Mar 30 2022, 2:04 AM

Address comments.

I just applied this to a recent HEAD and got a few warnings. Please make sure there are no new warnings on changes / new files.

/home/rengolin/devel/llvm-project/llvm/lib/Target/CSKY/CSKYInstrFormats.td:658:62: warning: unused template argument: R_Z_2:pattern
class R_Z_2<bits<6> sop, bits<5> pcode, string op, list<dag> pattern>
                                                             ^
In file included from /home/rengolin/devel/llvm-project/llvm/lib/Target/CSKY/CSKYFrameLowering.cpp:14:
/home/rengolin/devel/llvm-project/llvm/lib/Target/CSKY/CSKYMachineFunctionInfo.h:21:20: warning: private field 'MF' is not used [-Wunused-private-field]
  MachineFunction &MF;
                   ^
/home/rengolin/devel/llvm-project/llvm/lib/Target/CSKY/CSKYInstrInfo.cpp:480:24: warning: unused variable 'MRI' [-Wunused-variable]
  MachineRegisterInfo &MRI = MBB.getParent()->getRegInfo();
                       ^
/home/rengolin/devel/llvm-project/llvm/lib/Target/CSKY/Disassembler/CSKYDisassembler.cpp:169:21: warning: unused function 'DecodesFPR128RegisterClass' [-Wunused-function]
static DecodeStatus DecodesFPR128RegisterClass(MCInst &Inst, uint64_t RegNo,
                    ^
/home/rengolin/devel/llvm-project/llvm/lib/Target/CSKY/Disassembler/CSKYDisassembler.cpp:199:21: warning: unused function 'DecodeGPRSPRegisterClass' [-Wunused-function]
static DecodeStatus DecodeGPRSPRegisterClass(MCInst &Inst, uint64_t RegNo,
                    ^
/home/rengolin/devel/llvm-project/llvm/lib/Target/CSKY/Disassembler/CSKYDisassembler.cpp:36:16: warning: private field 'inDataRegion' is not used [-Wunused-private-field]
  mutable bool inDataRegion = false;
               ^

etc.

There is also a test error:

******************** TEST 'Clang :: Driver/csky-toolchain.c' FAILED ********************
...
/home/rengolin/devel/llvm-project/clang/test/Driver/csky-toolchain.c:16:24: error: C-CSKY-LINUX-MULTI: expected string not found in input
// C-CSKY-LINUX-MULTI: "{{.*}}/Inputs/multilib_csky_linux_sdk/lib/gcc/csky-linux-gnuabiv2/6.3.0/../../..{{/|\\\\}}..{{/|\\\\}}csky-linux-gnuabiv2/bin{{/|\\\\}}ld"
                       ^

I'm guessing this is the path of a local sysroot you have on your machine?

If possible, try to get a new environment (container, VM, alternative machine) and make sure a clean build still passes the tests.

zixuan-wu added a comment.EditedMar 30 2022, 7:34 PM

There is also a test error:

******************** TEST 'Clang :: Driver/csky-toolchain.c' FAILED ********************
...
/home/rengolin/devel/llvm-project/clang/test/Driver/csky-toolchain.c:16:24: error: C-CSKY-LINUX-MULTI: expected string not found in input
// C-CSKY-LINUX-MULTI: "{{.*}}/Inputs/multilib_csky_linux_sdk/lib/gcc/csky-linux-gnuabiv2/6.3.0/../../..{{/|\\\\}}..{{/|\\\\}}csky-linux-gnuabiv2/bin{{/|\\\\}}ld"

                       ^

I'm guessing this is the path of a local sysroot you have on your machine?

If possible, try to get a new environment (container, VM, alternative machine) and make sure a clean build still passes the tests.

I have met this before because the downloading of patch will ignore empty files. You can have a check that your apply does not contain new empty files in multilib_csky_linux_sdk.

For warnings, those are not introduced by this patch. I create another NFC to remove unused variable and function.

rengolin accepted this revision.Mar 31 2022, 3:35 AM

I have met this before because the downloading of patch will ignore empty files. You can have a check that your apply does not contain new empty files in multilib_csky_linux_sdk.

You are correct, it doesn't. :(

For warnings, those are not introduced by this patch. I create another NFC to remove unused variable and function.

Fair enough.

LGTM, thanks!

This revision is now accepted and ready to land.Mar 31 2022, 3:35 AM
This revision was landed with ongoing or failed builds.Apr 5 2022, 8:38 PM
This revision was automatically updated to reflect the committed changes.
dyung added a subscriber: dyung.Apr 5 2022, 9:21 PM

Hi, one of the tests you added. csky-toolchain.c seems to be failing on a Windows build bot, and from a quick look, it appears to be a path separator issue. Can you take a look?

https://lab.llvm.org/buildbot/#/builders/216/builds/2452

Hi, one of the tests you added. csky-toolchain.c seems to be failing on a Windows build bot, and from a quick look, it appears to be a path separator issue. Can you take a look?

https://lab.llvm.org/buildbot/#/builders/216/builds/2452

Reference to other target toolchain case, it should add unsupported directive. Fixed it at https://reviews.llvm.org/rG9906d38252d112894f304ba1b4fbdcd2cc93ab19

MaskRay added a comment.EditedSep 29 2022, 11:29 AM

mips computed sysroot from GCCInstallation very early in 2013 rG08450bd55ccdc4aee4f5f73cde97e25b3c4ce5b9 and Android followed up in 2018 (D45291), but I am not sure this is the correct direction.
They are to support very special distributions. Hard coding the file hierarchy for every Linux distribution just does not scale.
I raised my concern on https://reviews.llvm.org/D134454#3824630 as well.

With https://reviews.llvm.org/D134337 (default configuration file) we should move the logic to use a default configuration file instead.

mips computed sysroot from GCCInstallation very early in 2013 rG08450bd55ccdc4aee4f5f73cde97e25b3c4ce5b9 and Android followed up in 2018 (D45291), but I am not sure this is the correct direction.
They are to support very special distributions. Hard coding the file hierarchy for every Linux distribution just does not scale.
I raised my concern on https://reviews.llvm.org/D134454#3824630 as well.

With https://reviews.llvm.org/D134337 (default configuration file) we should move the logic to use a default configuration file instead.

It's fine for me to use config file. I only have 2 points.

  1. I agree sysroot should be separated from GCC because sysroot is not dependent to GCC and there is even not gcc when we use llvm runtime. This rule should also apply to multilib logic. Sysroot can detect multilib path individually.
  2. From user view, it's not easy to add sysroot or config file path manually, so it needs good way to enable this functionality.