This is an archive of the discontinued LLVM Phabricator instance.

[AIX][PowerPC] Remove error when specifying mabi=vec-default on AIX
ClosedPublic

Authored by ZarkoCA on May 7 2021, 3:27 PM.

Details

Summary

The default Altivec ABI was implemented but the clang error for specifying
its use still remains. Users could get around this but not specifying the
type of Altivec ABI but we need to remove the error.

Diff Detail

Event Timeline

ZarkoCA created this revision.May 7 2021, 3:27 PM
ZarkoCA requested review of this revision.May 7 2021, 3:27 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 7 2021, 3:27 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
jansvoboda11 added inline comments.
clang/lib/Frontend/CompilerInvocation.cpp
1861–1862

Can we simplify this to Opts.EnableAIXExtendedAltivecABI = O.matches(OPT_mabi_EQ_vec_extabi);?

ZarkoCA updated this revision to Diff 344078.May 10 2021, 8:58 AM
ZarkoCA edited the summary of this revision. (Show Details)
  • Simplify option logic
ZarkoCA updated this revision to Diff 344080.May 10 2021, 9:00 AM
  • Fix previous diff
ZarkoCA marked an inline comment as done.May 10 2021, 9:01 AM
ZarkoCA added inline comments.
clang/lib/Frontend/CompilerInvocation.cpp
1861–1862

Thanks, that works well.

LGTM...I'll approve this change unless there are any objections by EOD tomorrow.

jsji added a reviewer: Restricted Project.Jun 21 2021, 1:29 PM
jsji added a subscriber: jsji.Jun 21 2021, 1:36 PM

The default Altivec ABI was implemented

Please include the patches or commits that implement the default ABI.

clang/lib/Driver/ToolChains/Clang.cpp
4825

Why we need to check this if the default is to set it to -mabi=vec-default in below else?

clang/test/Driver/aix-vec-extabi.c
2

Why we need to explicitly DFLTABI-NOT: "-mabi=vec-default"

ZarkoCA updated this revision to Diff 353721.Jun 22 2021, 11:10 AM
ZarkoCA marked an inline comment as done.

Addressed comments:

  • Removed redundant elseif and CHECK
ZarkoCA marked 2 inline comments as done.Jun 22 2021, 11:12 AM
ZarkoCA added inline comments.
clang/lib/Driver/ToolChains/Clang.cpp
4825

I think my reasoning was to map -mabi=vec-default to something specific but we don't need it.

clang/test/Driver/aix-vec-extabi.c
2

It's not required, removed.

jsji added inline comments.Jun 22 2021, 11:30 AM
clang/test/Driver/aix-vec-extabi.c
0

How about RUN line without -mabi?
and without -maltivec?
and -mabi=vec-extabi without -maltivec?

ZarkoCA updated this revision to Diff 353748.Jun 22 2021, 12:23 PM
ZarkoCA marked 2 inline comments as done.
  • Updated test case
ZarkoCA added inline comments.Jun 22 2021, 12:28 PM
clang/test/Driver/aix-vec-extabi.c
0

Thanks, added those. Since -mabi-vec-extabi is a codegen option I think it makes sense to make sure it's not active when only -maltivec or nothing is specified on the command line. Does that work?

jsji accepted this revision as: jsji.Jun 22 2021, 12:35 PM

LGTM.

clang/test/Driver/aix-vec-extabi.c
0

I think you meant -vec-extabi option for llc? So maybe --implicit-check-not=vec-extabi should be better -- cover both mabi= and llc option.

2

Why not use --implicit-check-not?

This revision is now accepted and ready to land.Jun 22 2021, 12:35 PM
ZarkoCA updated this revision to Diff 353787.Jun 22 2021, 2:11 PM

Use --implicit-check-not

ZarkoCA marked an inline comment as done.Jun 22 2021, 2:12 PM
ZarkoCA added inline comments.
clang/test/Driver/aix-vec-extabi.c
2

Thank you, I wasn't aware of this option before this.

jsji added inline comments.Jun 22 2021, 2:22 PM
clang/test/Driver/aix-vec-extabi.c
1

--implicit-check-not=vec-extabi maybe better?

ZarkoCA updated this revision to Diff 353806.Jun 22 2021, 3:09 PM
ZarkoCA marked an inline comment as done.

check for vec-extabi instead of -mabi=vec-extabi

--implicit-check-not=vec-extabi maybe better?

@jsji had to rename the test file because the filename was tripping the error 😆 but this is better.

This revision was landed with ongoing or failed builds.Jun 23 2021, 4:41 AM
This revision was automatically updated to reflect the committed changes.