Page MenuHomePhabricator

[clang][AIX] Add option to control quadword lock free atomics ABI on AIX
ClosedPublic

Authored by lkail on Jun 7 2022, 1:15 AM.

Details

Summary

We are supporting quadword lock free atomics on AIX. For the situation that users on AIX are using a libatomic that is lock-based for quadword types, we can't enable quadword lock free atomics by default on AIX in case user's new code and existing code accessing the same shared atomic quadword variable, we can't guarentee atomicity. So we need an option to enable quadword lock free atomics on AIX, thus we can build a quadword lock-free libatomic(also for advanced users considering atomic performance critical) for users to make the transition smooth.

Diff Detail

Event Timeline

lkail created this revision.Jun 7 2022, 1:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 7 2022, 1:15 AM
lkail requested review of this revision.Jun 7 2022, 1:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 7 2022, 1:15 AM
lkail edited the summary of this revision. (Show Details)Jun 7 2022, 1:16 AM
lkail edited the summary of this revision. (Show Details)
lkail edited the summary of this revision. (Show Details)Jun 7 2022, 1:24 AM

Gentle ping.

amyk added a subscriber: amyk.Jun 28 2022, 8:12 PM

This patch makes sense. I had some questions regarding it.

clang/include/clang/Driver/Options.td
3642

Would it be better if we called this maix64-quadword-atomics instead?

clang/test/Driver/ppc-unsupported.c
12

Should we have a big endian Linux check, too?

amyk added inline comments.Jun 28 2022, 8:14 PM
clang/test/Driver/ppc-unsupported.c
12

Oh, sorry. I noticed there wasn't powerpc64-unknown-linux but I realized I think powerpc64-unknown-freebsd is supposed to be the big endian 64-bit Linux check, right?

lkail added inline comments.Jun 28 2022, 8:38 PM
clang/test/Driver/ppc-unsupported.c
12

Added OS check is following lines above. I'm ok to add powerpc64-unknown-linux too. Thanks for pointing it out.

lkail updated this revision to Diff 440860.Jun 28 2022, 10:48 PM

Address comments.

I am ok with this change overall, I just have a couple of questions about naming of the option.

  1. Is there any precedent for options that start with -maix or -m<OS> for any other OS?
  2. Is quadword the best word to use? There is no type information and this is restricted to integers. Would something like -maix-i128-atomics be a better name?
  3. Since this is kind of an ABI-related decision, would it make sense (and would it be possible) to make this a further suboption to the -mabi option? Something like -mabi=vec-extabi,i128-atomics,ieeelongdouble
lkail added a comment.Jun 29 2022, 1:29 AM

Is there any precedent for options that start with -maix or -m<OS> for any other OS?

There is -maix-struct-return.

Is quadword the best word to use? There is no type information and this is restricted to integers. Would something like -maix-i128-atomics be a better name?

'quadword' is used in ISA manual, so I follow it. Not merely i128, some struct types are also included, like

struct Q {
  char c[16];
};

Since this is kind of an ABI-related decision, would it make sense (and would it be possible) to make this a further suboption to the -mabi option?

This makes sense. I'll try this solution.

lkail updated this revision to Diff 441293.Jun 30 2022, 1:00 AM

Option name changed to -mabi=quadword-atomics as nemanja suggested.

Gentle ping.

Can we use the feature bit FeatureQuadwordAtomic to decide whether QuadAtomic is supported or not on AIX? Like what we do for Linux.

The reason we need this option is: we may need to compile a lock free libatomic on a Power7 or below target? If so, do we have similar issue on Linux? Thanks.

clang/include/clang/Driver/Options.td
3642

Do we need to change the backend check below too?

bool PPCTargetLowering::shouldInlineQuadwordAtomics() const {
  // TODO: 16-byte atomic type support for AIX is in progress; we should be able
  // to inline 16-byte atomic ops on AIX too in the future.
  return Subtarget.isPPC64() &&
         (EnableQuadwordAtomics || !Subtarget.getTargetTriple().isOSAIX()) &&
         Subtarget.hasQuadwordAtomics();
}
clang/lib/Basic/Targets/PPC.cpp
854

Can we set MaxAtomicInlineWidth in PPC64TargetInfo::setMaxAtomicWidth()? There is a TODO there

Can we use the feature bit FeatureQuadwordAtomic to decide whether QuadAtomic is supported or not on AIX? Like what we do for Linux.

FeatureQuadwordAtomic is for cpu level control, while -mabi=quadword-atomics is for ABI level. AIX running on pwr8+ also features FeatureQuadwordAtomic, but in the case described in the patch summary, sometimes we can't enable quadword lock free atomics on AIX by default, so that clang generate libcalls into libatomic rather than inlining lock free operations. libatomic has the final decision to use lock-free version or not.

The reason we need this option is: we may need to compile a lock free libatomic on a Power7 or below target?

We need to compile a quadword lock free libatomic for CPUs above pwr8.

If so, do we have similar issue on Linux? Thanks.

On Linux, clang is linking against GNU's libatomic by default, that depends on GNU libatomic's behaviour.

lkail added inline comments.Jul 21 2022, 10:38 PM
clang/include/clang/Driver/Options.td
3642

We don't need to change this yet. When we are compiling a quadword lock free libatomic, we use options -mabi=quadword-atomics -mllvm -ppc-quadword-atomics to enforce generating quadword lock-free code on AIX.

clang/lib/Basic/Targets/PPC.cpp
854

The TODO marks our roadmap towards enabling quardword lock free atomics on AIX too. Putting adjustment here is implementation reason: we don't context of LanguageOptions in PPC64TargetInfo::PPC64TargetInfo.

Thanks for the explanation. I am more clear now about the background.

clang/include/clang/Driver/Options.td
3642

This makes me confuse. We need to two different options to control the frontend and backend behavior?

Is it the final usage? Or we will add a follow up patch to map the backend one to the FE one? IMO finally we only need the driver option -mabi=quadword-atomics to control the final code generation for 128 bit atomic operations, right?

clang/lib/Basic/Targets/PPC.cpp
854

OK, fair enough.

lkail added inline comments.Jul 24 2022, 6:32 PM
clang/include/clang/Driver/Options.td
3642

This makes me confuse. We need to two different options to control the frontend and backend behavior?

This is multi-lang support consideration. clang is not the only frontend we have using LLVM as backend on AIX. If other language frontend generates store atomic i128, ..., the backend is supposed to generate libcalls into libatomic currently.

Is it the final usage?

No. We finally want to achieve -mabi=quadword-atomics by default and generate inline atomic code for cpu above pwr7 by default(no need to take OS into consideration).

shchenz added inline comments.Jul 24 2022, 9:47 PM
clang/include/clang/Driver/Options.td
3642

I know what you mean. But I assume the driver option -mabi=quadword-atomics will impact the assembly instead of just impact the frontend, right? Using -mllvm option is not right as the final solution.

There are some driver options example, like -gstrict-dwarf, Frontend can control the backend behavior and the backend can also change this option by -strict-dwarf.

Could you please explain:
1: how the backend will handle -mabi=quadword-atomics in future?
2: on what condition, we can start to remove below TODOs:

bool PPCTargetLowering::shouldInlineQuadwordAtomics() const {
// TODO: 16-byte atomic type support for AIX is in progress;
}
PPC64TargetInfo::setMaxAtomicWidth() {
    // TODO: We should allow AIX to inline quadword atomics in the future.
}
lkail added inline comments.Jul 24 2022, 9:58 PM
clang/include/clang/Driver/Options.td
3642

1, 2 can be answered together. After we ship new libatomic to users(I assume that that isn't in near future, since this requires AIX OS upgrade), we can enable quadword lock free atomics in both clang and llvm backend by default. Backend isn't aware of -mabi=quadword-atomics. This option changes layout of quadword atomic types(align to 16 byte), and generate atomic LLVM IR rather than generating libcalls in LLVM IR.

shchenz accepted this revision as: shchenz.Jul 24 2022, 11:15 PM

LGTM. Thanks.

This revision is now accepted and ready to land.Jul 24 2022, 11:15 PM
This revision was landed with ongoing or failed builds.Jul 26 2022, 6:56 PM
This revision was automatically updated to reflect the committed changes.