[PATCH] D75866: [AIX] supporting the visibility attribute for aix assembly

[Hubert Tong via Phabricator via llvm-commits]

hubert.reinterpretcast added inline comments.


================
Comment at: llvm/include/llvm/BinaryFormat/XCOFF.h:162
 
+enum VisibilityType : uint16_t {
+  SYM_V_UNSPECIFIED = 0x0000,
----------------
A lot of the comments in the file should be Doxygen-style, but the relative dearth of them is no reason not to use one here.

We should have a comment:
Values for visibility as they would appear when encoded in the high 4 bits of the 16-bit unsigned n_type field of symbol table entries. Valid for 32-bit XCOFF only when the vstamp in the auxiliary header is greater than 1.



================
Comment at: llvm/include/llvm/MC/MCStreamer.h:568
 
+  /// Emit a symbol's linkage and visibilty in linkage directive for XCOFF.
+  ///
----------------
s/in linkage directive/with a linkage directive/;


================
Comment at: llvm/include/llvm/MC/MCStreamer.h:572
+  /// \param Linkage - The linkage of the symbol to emit.
+  /// \param Visibility - The visibility of the symbol to emit. 
+  virtual void emitXCOFFSymbolLinkageWithVisibility(MCSymbol *Symbol,
----------------
jasonliu wrote:
> nit, we need to mention it here as Doxygen style:
> When the symbol do not have any visibility setting, pass in `MCSA_Invalid`.
Suggestion:
... to emit or `MCSA_Invalid` is the symbol does not have an explicit visibility.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:1510
     // Function getSymbol gives us the function descriptor symbol for XCOFF.
-    if (TM.getTargetTriple().isOSBinFormatXCOFF() && !F.isIntrinsic()) {
+    if (TM.getTargetTriple().isOSBinFormatXCOFF()) {
+      if (F.isIntrinsic())
----------------
I would suggest flipping this:
```
if (!TM.getTargetTriple().isOSBinFormatXCOFF()) {
  // ...
  continue;
}

if (F.isIntrinsic())
  continue;

// ...
```


================
Comment at: llvm/lib/MC/MCAsmInfoXCOFF.cpp:17
   IsLittleEndian = false;
+  HasVisibilityOnlyWithLinkage = true;
   SupportsQuotedNames = false;
----------------
This should be rebased for new changes.


================
Comment at: llvm/lib/MC/MCAsmStreamer.cpp:821
+  case MCSA_Invalid:
+    break;
+  case MCSA_Hidden:
----------------
Should there be a comment like
```
// Nothing to do.
```
?


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1580
+  assert(MAI->hasVisibilityOnlyWithLinkage() &&
+         "AIX's linkage directive have visibility setting.");
+
----------------
s/directive have visibility/directives take a visibility/;


================
Comment at: llvm/test/CodeGen/PowerPC/aix-xcoff-hidden.ll:1
+; RUN: llc -verify-machineinstrs  -mtriple powerpc-ibm-aix-xcoff -mcpu=pwr4 -mattr=-altivec < %s | \
+; RUN:   FileCheck %s
----------------
Please fix the two consecutive spaces before `-mtriple`.


================
Comment at: llvm/test/CodeGen/PowerPC/aix-xcoff-hidden.ll:3
+; RUN:   FileCheck %s
+; RUN: llc -verify-machineinstrs  -mtriple powerpc64-ibm-aix-xcoff -mcpu=pwr4 -mattr=-altivec < %s |\
+; RUN:   FileCheck %s
----------------
Same comment.


================
Comment at: llvm/test/CodeGen/PowerPC/aix-xcoff-hidden.ll:9
+
+define void @foo()  {
+entry:
----------------
Fix the two spaces before the closing brace.


================
Comment at: llvm/test/CodeGen/PowerPC/aix-xcoff-hidden.ll:14
+
+define  hidden void @foo_h(i32* %ip) {
+entry:
----------------
Fix the two spaces after `define`.


================
Comment at: llvm/test/CodeGen/PowerPC/aix-xcoff-hidden.ll:19
+
+define  protected void @foo_protected(i32* %ip) {
+entry:
----------------
Same comment.


================
Comment at: llvm/test/CodeGen/PowerPC/aix-xcoff-hidden.ll:43
+
+; CHECK:        .globl  foo[DS]
+; CHECK:        .globl  .foo
----------------
This does not actually prevent `,something` from being on this line.
Add `{{[[:space:]]*([#].*)?$}}`.


================
Comment at: llvm/test/CodeGen/PowerPC/aix-xcoff-hidden.ll:44
+; CHECK:        .globl  foo[DS]
+; CHECK:        .globl  .foo
+; CHECK:        .globl  foo_h[DS],hidden
----------------
Same comment.


================
Comment at: llvm/test/CodeGen/PowerPC/aix-xcoff-hidden.ll:47
+; CHECK:        .globl  .foo_h,hidden
+; CHECK:        .globl  foo_protected[DS],protected
+; CHECK:        .globl  .foo_protected,protected
----------------
The file is called `aix-xcoff-hidden.ll`? Should it be renamed?


================
Comment at: llvm/test/CodeGen/PowerPC/aix-xcoff-hidden.ll:54
+; CHECK:        .globl  b_h,hidden
+; CHECK:        .weak   zoo_weak_extern_h[DS],hidden
+; CHECK:        .extern .bar_h,hidden
----------------
Does it make sense to have a blank line between the defined variables and the undefined symbols?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75866