Page MenuHomePhabricator

[AIX] Add mabi=vec-extabi options to enable the AIX extended and default vector ABIs.
ClosedPublic

Authored by ZarkoCA on Oct 19 2020, 3:27 AM.

Details

Summary

Added support for the options mabi=vec-extabi and mabi=vec-default which are analogous to qvecnvol and qnovecnvol when using XL on AIX.
The extended Altivec ABI on AIX is enabled using mabi=vec-extabi in clang and vec-extabi in llc.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Xiangling_L added inline comments.Oct 19 2020, 9:01 AM
clang/lib/CodeGen/BackendUtil.cpp
557

The ABI specifies When the option to use nonvolatile vector registers is enalbed. the compilation environment must also predefine __EXTABI__. I didn't see this. Should we also cover this in this patch?

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

I would suggest s/haveMaltivec/HasAltivec to be consistent with other places where if altivec enabled is tested.

4624

Any reason why we cannot use Args.getLastArg here for OPT_maltivec as well?

4634

Since we are defaulting to default altivec ABI, so I think the logic here should be if (HasAltivec && !Args.getLastArg(options::OPT_mvecnvol)), then we emit D.Diag(diag::err_aix_default_altivec_abi) error?

clang/test/CodeGen/altivec.c
57

Could we also add a testcase to test -mvecnvol/-mnovecnvol are AIX only options?

There's frontend changes anyway, so there should be changes for the predefined macros?

Xiangling_L added inline comments.Oct 19 2020, 1:03 PM
llvm/lib/CodeGen/CommandFlags.cpp
525

Should we also check -vecnvol option is used for AIX only somewhere?

llvm/lib/Target/PowerPC/PPCISelLowering.cpp
6973

minor:
remove '.'

llvm/test/CodeGen/PowerPC/aix-AppendingLinkage.ll
4 ↗(On Diff #298986)

May I ask why would we want to add -vecnvol for those testcases? As I noticed, they don't need altivec feature enabled.

I have two concerns over the choice of the option form/name.

Firstly, there's an -mabi= option that is used to select for ABI extensions. The choice of using an -mabi= suboption for selecting between the extended vector ABI and the default vector ABI should be considered.
Secondly, the vecnvol name does not allude to its relation with the vector-extended ABI.

-mabi=aixvecextabi was suggested when I discussed this with someone who is familiar with GCC. The choice of extabi at the end is to match the spelling of the __EXTABI__ macro.

-mabi=aixvecextabi was suggested when I discussed this with someone who is familiar with GCC. The choice of extabi at the end is to match the spelling of the __EXTABI__ macro.

To reduce the length of the option, and with the hopes that future vector-related ABI extensions on Power have more specific naming than just being the "extended" ABI, the updated suggestion is -mabi=vec-extabi.

DiggerLin added inline comments.
clang/docs/ClangCommandLineReference.rst
2890

I think it should be

.. option:: -mvecnvol, -mnovecnvol
  Only supported On AIX. Specify usage of volatile and nonvolatile vector registers, the extended vector ABI on AIX. Defaults to '-mnovecnvol' when Altivec is enabled.
clang/lib/Driver/ToolChains/Clang.cpp
4634

I think we do not need a new variable here. we can write as
if (A->getOption().matches(options::OPT_mnovecnvol) && Args.getLastArg(OPT_maltivec)

ZarkoCA updated this revision to Diff 305136.EditedFri, Nov 13, 7:30 AM
ZarkoCA retitled this revision from [AIX] Add mvecnvol and mnovecnvol options to enable the AIX extended and default vector ABIs. to [AIX] Add mabi=vec-extabi options to enable the AIX extended and default vector ABIs. .
ZarkoCA edited the summary of this revision. (Show Details)

Addressed comments:

  1. Changed name of options to mabi=vec-extabi and mabi=vec-default
  2. Reworked logic for selecting the options in Clang.cpp
  3. Added mattr=-altivec instead of using mabi-vec-extabi to tests that failed previously.
ZarkoCA marked 8 inline comments as done.Fri, Nov 13, 7:54 AM
ZarkoCA added inline comments.
clang/lib/CodeGen/BackendUtil.cpp
557

Thanks, that was an oversight.

clang/lib/Driver/ToolChains/Clang.cpp
4614–4631

I think what XL does is probably the correct thing but in clang/llvm it looks like the hasAltivec setting is determined by the cpu level and the compiler simply ignores it when it's not supported by the cpu.

For now, I'd like to follow the existing logic as all the other PPC targets and then I can follow up with a patch that emits an error when selecting altivec when the cpu doesn't support it.

4622

I reworked this so that I hopefully remove any confusion.

llvm/lib/CodeGen/CommandFlags.cpp
525

Is there a way to check whether an llc option is target specific?

llvm/test/CodeGen/PowerPC/aix-AppendingLinkage.ll
4 ↗(On Diff #298986)

It is odd but those test cases hit the error llvm/lib/Target/PowerPC/PPCISelLowering.cpp:6908 when that wasn't enabled. However, adding mattr=-altivec also suppresses it. It seems like specifying mcpu=pwr4 doesn't not completely remove all altivec opts?

Xiangling_L added inline comments.Wed, Nov 18, 2:52 PM
clang/lib/Driver/ToolChains/Clang.cpp
4616

I am wondering what cases are not covered by Triple.isOSAIX()? Why do we also query Triple.isOSBinFormatXCOFF()?

4617

I see your intention here is to find if there are any -mabi=vec-extabi or -mabi=vec-default option specifying. If I am correct, why we need to loop through whole Args? Can we just use hasArg query?

clang/lib/Frontend/CompilerInvocation.cpp
1438

ditto.

clang/test/CodeGen/altivec.c
2–18

Based on the code added in BackendUtil:551, should we also add a case for compiling a source to assembly?

llvm/include/llvm/Target/TargetMachine.h
246 ↗(On Diff #305136)

I don't see this function gets invoked anywhere,can we remove it?

llvm/include/llvm/Target/TargetOptions.h
179

-mabi=vec-extabi is the FE option, should we s/-mabi=vec-extabi/-vec-extabi?

llvm/lib/CodeGen/CommandFlags.cpp
286

Based on the naming convention in this file context, this seems should be EnableAIXExtendedAltivecABI.

287

Can we add a testcase for this backend option?

llvm/test/CodeGen/PowerPC/aix-default-vec-abi.ll
1 ↗(On Diff #305136)

Can we simplify the testcase to only contain one vector type parameter? I think that would be sufficient to trigger the error.

4 ↗(On Diff #305136)

ditto: can we simplify the testcase?

llvm/test/CodeGen/PowerPC/aix-xcoff-mergeable-str.ll
7 ↗(On Diff #305136)

I am wondering why do we remove -data-sections=false here?

This revision is now accepted and ready to land.Thu, Nov 19, 6:07 AM
ZarkoCA updated this revision to Diff 306509.Thu, Nov 19, 12:45 PM
ZarkoCA marked 3 inline comments as done.

Addressed comments:
Added and fixed test cases and changed option selection logic.

ZarkoCA marked 10 inline comments as done.Thu, Nov 19, 12:50 PM
ZarkoCA added inline comments.
clang/lib/Driver/ToolChains/Clang.cpp
4616

The path isn't selected if someone were to select -powerpc-unknown-xcoff as a target for example. It looks like the Triple.isOSAIX() is true when we we have aix in the target triple.

clang/test/CodeGen/altivec.c
2–18

Added in lines 15-18

llvm/include/llvm/Target/TargetOptions.h
179

Good catch, fixed.

llvm/lib/CodeGen/CommandFlags.cpp
287

Added in llvm/test/CodeGen/PowerPC/aix-vec-abi.c

Xiangling_L added inline comments.Thu, Nov 19, 5:43 PM
clang/lib/Driver/ToolChains/Clang.cpp
4616

The path isn't selected if someone were to select -powerpc-unknown-xcoff as a target for example. It looks like the Triple.isOSAIX() is true when we we have aix in the target triple.

My understanding is that the AIX altivec ABI is target-dependent, it has nothing to do with binary format.

4617

The common query Args.hasArg(options::OPT_maltivec) is better to put in an upper level if;
Also after you do that, the last else if can be replaced by a else;
Another issue is that when we on AIX with -maltivec + extended/defautlt altivec abi enabled, we do duplicate checking in #4615 & #4613 if blocks.

Maybe we can refactor these two if blocks into:

if (Arg *A = Args.getLastArg(options::OPT_mabi_EQ_vec_extabi,
                             options::OPT_mabi_EQ_vec_default)) {
  if (!Triple.isOSAIX()) {
    D.Diag(diag::err_drv_unsupported_opt_for_target)
        << A->getSpelling() << RawTriple.str();
  } else {
    if (Args.hasArg(options::OPT_maltivec)) {
      if (Args.hasArg(options::OPT_mabi_EQ_vec_extabi)) {
        CmdArgs.push_back("-mabi=vec-extabi");
      } else if (Args.hasArg(options::OPT_mabi_EQ_vec_default)) {
        D.Diag(diag::err_aix_default_altivec_abi);
      } else {
        D.Diag(diag::err_aix_default_altivec_abi);
      }
    } else {
      D.Diag(diag::err_aix_altivec);
    }
  }
}
4633

ditto.

clang/lib/Frontend/CompilerInvocation.cpp
1437

Should we also check if target feature altivec[-target-feature +altivec] is enabled when using these two options? If so, we should also add related testcases.

1439

ditto.

clang/test/CodeGen/altivec.c
2–18

It looks line 2& 4, line 3&5 are duplicated.

2–18

Added in lines 15-18

Sorry, I should make my point clearer. Based on current testcases, there are two things:

  1. line 15-18 are actually duplication to 10,11, 13. 14. Because all of them are testing if the driver will emit error when not specifying -maltivec with -mabi=vec-default/-mabi=vec-extabi, i.e compiling from .c to .ll and .c to .s won't affect how driver works,
  1. BackendUtil:551 The code I mentioned is actually affecting how BE behaves when we enable AIX altivec in the FE[or driver]. So the testcase I am looking for is something like:
// RUN:  %clang -target powerpc-unknown-aix -S -maltivec -mabi=vec-extabi %s  | FileCheck  %s
// CHECK: LLVM ERROR: the extended Altivec AIX ABI is not yet supported
llvm/test/CodeGen/PowerPC/aix-func-dsc-gen.ll
2 ↗(On Diff #306509)

I am not sure if this is for all testcases where you add -mattr=-altivec, but I tried the first three. They all passed without this option. Could you double check this?

llvm/test/CodeGen/PowerPC/aix-vec-abi.ll
2

May I ask why we use pwr8 for this test?

ZarkoCA marked 5 inline comments as done.Fri, Nov 20, 8:03 AM
ZarkoCA added inline comments.
clang/lib/Frontend/CompilerInvocation.cpp
1437

Both of these options require that -maltivec is also selected which sets -target-feature +altivec.

clang/test/CodeGen/altivec.c
2–18

As far as I understand, testing the assembly path is a bit tricky mainly due to how Altivec is determined to be on for all powerpc targets.

The tests in this file won't trigger the Altivec ABI errors because they are calling convention ABIs and there is no parameter passing or returns. In fact, they will generate assembly because the default CPU has the Altivec attribute enabled.

I wrote tests that will use the Altivec calling convention ABI in those cases we trigger earlier errors such as "vector type is unimplemented on AIX".

But, I did a test which shows that the driver passes the -mabi=vec-extabi option.

llvm/test/CodeGen/PowerPC/aix-func-dsc-gen.ll
2 ↗(On Diff #306509)

You're right,it doesn't look like it's needed any longer.

ZarkoCA updated this revision to Diff 306696.Fri, Nov 20, 8:09 AM
ZarkoCA marked an inline comment as done.

Addressed comments and added a test to check whether the driver passes these options.

Xiangling_L added inline comments.Fri, Nov 20, 12:52 PM
clang/lib/Frontend/CompilerInvocation.cpp
1437

Both of these options require that -maltivec is also selected which sets -target-feature +altivec.

It seems the scenario you pointed out are driver invocation use case. But when users invoke clang -cc1 not driver with -mabi=ext-abi/default, should we also check if the user specify -target-feature +altivec as well? For now, based on my observation, if invoking cc1 without -target-feature +altivec but with -mabi=ext-abi only like // RUN: %clang_cc1 -mabi=vec-extabi -triple powerpc64-unknown-aix -emit-llvm %s -o - | FileCheck %s, the error is error: unknown type name 'vector'. Should we explicitly say -target-feature +altivec is required as what we do for driver?

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

minor: We can omit --check-prefix=CHECK

llvm/include/llvm/Target/TargetOptions.h
179

minor:
s/AIXExtendedAltivecABI/EnableAIXExtendedAltivecABI

ZarkoCA updated this revision to Diff 307192.Mon, Nov 23, 1:46 PM
ZarkoCA marked 3 inline comments as done.

Addressed some of the comments.

Xiangling_L added inline comments.Tue, Nov 24, 7:40 AM
clang/include/clang/Basic/LangOptions.def
186

minor nit: remove extra spaces in front of 1.

clang/lib/Frontend/CompilerInvocation.cpp
1439

I am wondering why do we add !T.isOSBinFormatXCOFF() back?

1443

Just a record here, as we discussed offline, we don't need to emit this error in frontend.

clang/test/CodeGen/altivec.c
7

When user specify -maltivec / -target-feature +altivec without using any abi option, the compiler will assume default altivec abi. In this situation, since default abi hasn’t been implemented, we should emit an error. So can we also add testcases for :

// RUN: not %clang -S -emit-llvm -maltivec -target powerpc-unknown-aix %s 2>&1 | FileCheck %s --check-prefix=AIX-ERROR
and
// RUN: not %clang_cc1 -target-feature +altivec -triple powerpc-unknown-aix -emit-llvm %s 2>&1 | FileCheck %s --check-prefix=AIX-ERROR
ZarkoCA updated this revision to Diff 307405.Tue, Nov 24, 10:24 AM

Went back to old option selection logic as updated version did not emit an error when selecting 'maltivec` but not mabi=vec-extabi.

Fixed formatting.

ZarkoCA marked 7 inline comments as done.Tue, Nov 24, 10:33 AM
ZarkoCA added inline comments.
clang/test/CodeGen/altivec.c
7

That's a good catch, the error was able to be generated previously but reworking the logic in Clang.cpp with the previous diff caused it to not be emitted. I went back to the older logic which emits the error in cases like where maltivec is specified without mabi=vec-extabi.

As for the cc1 error, currently the not vector types error catch that before we emit the Altivec ABI error since at this time there isn't a good way to check for target-feature +altivec in cc1.

llvm/test/CodeGen/PowerPC/aix-vec-abi.ll
2

Sorry for missing this earlier, I wanted to specify a CPU that has Altivec instructions enabled so that hasAltivec true without the user specifying it.

Xiangling_L added inline comments.Tue, Nov 24, 1:01 PM
clang/lib/Driver/ToolChains/Clang.cpp
4624

line 4624 to line 4635 can be simplified to :

if (Triple.isOSAIX() && Args.hasArg(options::OPT_maltivec) {
  if (Args.hasArg(options::OPT_mabi_EQ_vec_extabi)) {
    CmdArgs.push_back("-mabi=vec-extabi");
  } else {
    D.Diag(diag::err_aix_default_altivec_abi);
  }
}

or even simplify line 4617 -4636 to the following if it works:

if (Arg *A = Args.getLastArg(options::OPT_mabi_EQ_vec_extabi,
                             options::OPT_mabi_EQ_vec_default)) {
  if (!Triple.isOSAIX())
    D.Diag(diag::err_drv_unsupported_opt_for_target)
        << A->getSpelling() << RawTriple.str();

  if (!Args.hasArg(options::OPT_maltivec))
    D.Diag(diag::err_aix_altivec);

  if (Args.hasArg(options::OPT_mabi_EQ_vec_default))
    D.Diag(diag::err_aix_default_altivec_abi);

  CmdArgs.push_back("-mabi=vec-extabi");
} else if (Triple.isOSAIX() && Args.hasArg(options::OPT_maltivec) {
    D.Diag(diag::err_aix_default_altivec_abi);
}
clang/lib/Frontend/CompilerInvocation.cpp
1439

Hi Zarko, is the above comment missed being addressed?

ZarkoCA updated this revision to Diff 307449.Tue, Nov 24, 2:04 PM

Simplified option logic as per suggestion.
Removed stray isXCOFF reference.

ZarkoCA marked 7 inline comments as done.Tue, Nov 24, 2:06 PM
ZarkoCA added inline comments.
clang/lib/Driver/ToolChains/Clang.cpp
4624

I really like the first suggestion, thank you.

clang/lib/Frontend/CompilerInvocation.cpp
1439

Sorry, that one snuck back in there after I thought I removed it.

Xiangling_L accepted this revision.Tue, Nov 24, 2:10 PM

LGTM. Thanks!