hubert.reinterpretcast added inline comments.
================
Comment at: clang/lib/Basic/Targets/PPC.h:448
+ void setMaxAtomicWidth() override {
+ // For layout on ELF targets, we support up to 16 bytes.
+ if (getTriple().isOSBinFormatELF())
----------------
hubert.reinterpretcast wrote:
> lkail wrote:
> > hubert.reinterpretcast wrote:
> > > I believe this should be presented more as this override being
> > > implemented currently only for ELF targets. The current presentation
> > > seems to imply more design intent for non-ELF targets than there is
> > > consensus for.
> > >
> > > For example:
> > > ```
> > > if (!getTriple().isOSBinFormatELF())
> > > return PPCTargetInfo::setMaxAtomicWidth();
> > > ```
> > It looks a chance to adjust according to arch level to me(Considering we
> > finally will make xcoff and elf targets consistent here). If you have
> > strong opinion on this, I'll make a change here.
> This function currently modifies two different things. One that should be
> arch-level-sensitive (and, iiuc, is already so) and one that should not be
> arch-level-sensitive.
>
> So, the `isOSBinFormatELF` checks are here mainly to scope the patch
> (temporarily).
>
> I only suggested to use the call to the base class as a shorthand for
> "nothing special that this function is doing". The associated comment should
> read something like "Restrict 16-byte atomic changes to ELF targets for now.
> TODO: Consider other targets as well."
>
> At the very least (for this patch), I do not believe the `isOSBinFormatELF`
> should be checked twice in terms of control flow.
The new version of the change already addresses this; thanks.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D122377/new/
https://reviews.llvm.org/D122377
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
