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

[Jason Liu via Phabricator via llvm-commits]

jasonliu added inline comments.


================
Comment at: llvm/include/llvm/MC/MCSymbolXCOFF.h:64
   MCSectionXCOFF *RepresentedCsect = nullptr;
+  XCOFF::SymbolVisibilityType VisibilityType = XCOFF::SYM_V_INTERNAL;
 };
----------------
This makes every symbol by default is `internal` visibility?
On ELF, the default visibility for symbol is `default`. On AIX, if visibility feature is turned on, the default is `unspecified`. 
Setting it to internal here might not be what we want. There is visibility `internal`. Try to implement visibility `unspecified` with `internal` is confusing.


================
Comment at: llvm/lib/MC/MCAsmStreamer.cpp:826
+        XCOFFSym->getRepresentedCsect()->getMappingClass() != XCOFF::XMC_PR)
+      QualName->print(OS, MAI);
+    else
----------------
Is there a place we passed in the wrong symbol?
I would expect we could do symbol printing directly instead of querying if a qualname need to be emitted.


================
Comment at: llvm/lib/MC/MCAsmStreamer.cpp:836
+  case MCSA_Hidden:
+    OS << ", hidden";
+    break;
----------------
We don't like spaces between commas: https://reviews.llvm.org/D80247


================
Comment at: llvm/test/CodeGen/PowerPC/aix-xcoff-hidden.ll:1
+; RUN: llc -verify-machineinstrs  -mtriple powerpc-ibm-aix-xcoff  < %s | FileCheck %s
+; RUN: llc -verify-machineinstrs  -mtriple powerpc64-ibm-aix-xcoff < %s | FileCheck %s
----------------
Missing mcpu settings (the default mcpu is not correct for AIX).


================
Comment at: llvm/test/CodeGen/PowerPC/aix-xcoff-hidden.ll:56
+; CHECK:        .globl  .foo
+; CHECK:        .globl  foo_h[DS], hidden
+; CHECK:        .globl  .foo_h, hidden
----------------
I'm only seeing `hidden` visibility attr and `unspecified` being test here.
But we could have 5 combination here with globl directive. I would like to list here so we could be clear:
1. no visibility attribute -> `unspecified`
2. exported -> `default`
3. hidden -> `hidden`
4. internal -> `internal`
5. protected -> `protected`

It's not clear from the patch description, how many of this visibility attribute we are going to support. If we don't say anything, I would assume we are going to support all of them. Then we are clearly missing some implementation and testing here. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75866