[PATCH] D127189: [clang][AIX] Add option to control quadword lock free atomics ABI on AIX

[Kai Luo via Phabricator via cfe-commits]

lkail added inline comments.


================
Comment at: clang/include/clang/Driver/Options.td:3611
   HelpText<"Enable the default Altivec ABI on AIX (AIX only). Uses only volatile vector registers.">;
+def maix_quadword_atomics : Flag<["-"], "maix64-quadword-atomics">,
+  Group<m_Group>, Flags<[CC1Option]>,
----------------
shchenz wrote:
> lkail wrote:
> > shchenz wrote:
> > > lkail wrote:
> > > > shchenz wrote:
> > > > > amyk wrote:
> > > > > > Would it be better if we called this `maix64-quadword-atomics` instead? 
> > > > > 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();
> > > > > }
> > > > > ```
> > > > 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.
> > > 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?
> > > 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).
> 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.
> }
> ```
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D127189/new/

https://reviews.llvm.org/D127189