[PATCH] D78929: [AIX][XCOFF]emit extern linkage for the llvm intrinsic symbol

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1756
+  bool Ret = AsmPrinter::doFinalization(M);
+  for (auto Sym : IntrinsicSymbols)
+    OutStreamer->emitSymbolAttribute(Sym, MCSA_Extern);
----------------
hubert.reinterpretcast wrote:
> DiggerLin wrote:
> > jhenderson wrote:
> > > Don't use `auto` here. Also, should this really be a copy and not a const reference?
> > it store the a pointer of MCSymbol in the IntrinsicSymbols. I think return a copy of pointer is same efficient as const reference. 
> > 
> > and the 
> > 
> > ```
> > bool MCXCOFFStreamer::emitSymbolAttribute(MCSymbol *Sym,
> >                                           MCSymbolAttr Attribute)
> > ```
> One issue with `auto` here is that it's not obvious that we have a pointer type (and even knowing how `Sym` is used does not rule out the possibility that it is merely convertible to the pointer type). Using `auto *` might not have caused the comment to be raised, but since the comment has been raised, please get rid of the `auto`.
By the way, there's a related style guide section on usage of `auto`: https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable


================
Comment at: llvm/test/CodeGen/PowerPC/aix-llvm-intrinsic.ll:2
+; RUN: llc -verify-machineinstrs -mtriple powerpc-ibm-aix-xcoff -mcpu=pwr4 \
+; RUN: -mattr=-altivec < %s | FileCheck --check-prefixes=COMMON,BIT32 %s
+
----------------
DiggerLin wrote:
> jhenderson wrote:
> > Please don't break the lines up like this. I'd put -mattr=altivec on the same line as the rest of the llc line, but if you do want to break it onto a new line, split it roughly halfway through the arguments, and indent the second line, then put the FileCheck line on a new line. Same goes below.
> thanks
One more related request: Indent the FileCheck directive by an extra two spaces to make it obvious it's a continuation of the previous line:

```
; RUN: llc ... | \
; RUN:   FileCheck ...
```

Same below.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78929