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())
----------------
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.


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

Reply via email to