This is an archive of the discontinued LLVM Phabricator instance.

[AIX] use system assembler for assembly files
ClosedPublic

Authored by shchenz on Apr 16 2023, 8:38 PM.

Details

Summary

Change to system assembler to compile assembly files even -fintegrated-as is specified.
We don't have a good Clang as for now for assembly files on AIX.

Diff Detail

Event Timeline

shchenz created this revision.Apr 16 2023, 8:38 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 16 2023, 8:38 PM
shchenz requested review of this revision.Apr 16 2023, 8:38 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 16 2023, 8:38 PM

gentle ping

qiucf added a subscriber: qiucf.Apr 24 2023, 7:14 PM
qiucf added inline comments.
clang/lib/Driver/ToolChains/AIX.cpp
427 ↗(On Diff #514083)

I saw we have a ToolChain::getAssemble() method. Can we use that directly?

clang/test/Driver/aix-assembler.s
2

Empty line?

shchenz updated this revision to Diff 517011.Apr 25 2023, 6:32 PM

address @qiucf comments

shchenz marked an inline comment as done.Apr 25 2023, 6:32 PM

gentle ping...

gentle ping...

MaskRay added inline comments.May 10 2023, 8:14 PM
clang/test/Driver/aix-assembler.s
23

I am not sure you need 6 RUN lines to test this. Whether a target uses integrated assembler has an existing test file and you can reuse that.

shchenz updated this revision to Diff 521233.May 11 2023, 2:31 AM

update tests

I am not sure you need 6 RUN lines to test this. Whether a target uses integrated assembler has an existing test file and you can reuse that.

I don't have strong prefer which one we should use, a new file or reuse file. Since you point out, I change to reuse a legacy one in same dir. Please notice that there are some other targets that has their own assembler test file, like mips-as.s, systemz-as.s, arm64-as.s.

I am not sure you need 6 RUN lines to test this. Whether a target uses integrated assembler has an existing test file and you can reuse that.

I don't have strong prefer which one we should use, a new file or reuse file. Since you point out, I change to reuse a legacy one in same dir. Please notice that there are some other targets that has their own assembler test file, like mips-as.s, systemz-as.s, arm64-as.s.

Thanks. It makes sense to check whether reusing a test file (possibly renaming it) makes sense.
https://maskray.me/blog/2021-08-08-toolchain-testing "an existing test can be enhanced"

Some existing tests can be improved. You may improve the testing without cargo culting:)

qiucf accepted this revision as: qiucf.May 30 2023, 9:35 PM

This looks reasonable to me, thanks.

This revision is now accepted and ready to land.May 30 2023, 9:35 PM
MaskRay added inline comments.May 31 2023, 8:12 AM
clang/test/Driver/target-as.s
1 ↗(On Diff #521233)

aix-as.c will be more appropriate.

shchenz added inline comments.May 31 2023, 10:19 PM
clang/test/Driver/target-as.s
1 ↗(On Diff #521233)

Do you mean we change this file to aix-as.s or we change back to previous version? The file target-as.s can not be renamed to aix-as.s, it also contains tests for i386, see the first run line.

MaskRay added inline comments.Jun 1 2023, 12:45 PM
clang/test/Driver/target-as.s
1 ↗(On Diff #521233)

You can add the new tests to the existing aix-as.s. The behavior is about assemblers on AIX. aix-as.s seems more appropriate than target-as.s.

shchenz added inline comments.Jun 1 2023, 7:07 PM
clang/test/Driver/target-as.s
1 ↗(On Diff #521233)

hmm, we want to use assembly file for test. There is aix-as.c but no aix-as.s.

MaskRay added inline comments.Jun 2 2023, 12:50 PM
clang/test/Driver/target-as.s
1 ↗(On Diff #521233)

You can use -x assembler. It's fairly common to use -x c++ xxx.c to test C++ with a C file.

This revision was automatically updated to reflect the committed changes.
brad added a subscriber: brad.Jul 9 2023, 8:13 PM

What is left for this to be reverted, in the quest to fully enable IAS for AIX?

What is left for this to be reverted, in the quest to fully enable IAS for AIX?

See XCOFFAsmParser::ParseDirectiveCSect() and I think qual symbol names in AIX assembly(quite common for AIX assembly) can also not be parsed with llvm's asm parser.