[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/lib/Target/PowerPC/PPCAsmPrinter.cpp:1587
+  GlobalValue::LinkageTypes Linkage = GV->getLinkage();
+  GlobalValue::VisibilityTypes Visibiltiy = GV->getVisibility();
+  MCSymbolAttr VisibilityAttr = MCSA_Invalid;
----------------
`Linkage` and "`Visibiltiy`" are used only once. I would suggest that we don't need these variables.


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1592
+  default:
+    break;
+  case GlobalValue::HiddenVisibility:
----------------
There should be a limited number of VisibilityTypes, make this a covering switch.


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1603
+  default:  
+    AsmPrinter::EmitLinkage(GV, GVSym);
+    return;
----------------
Why is it okay to ignore visibility when encountering this case?


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1606
+  case GlobalValue::ExternalLinkage:
+    // If external, declare as a global symbol: .globl _foo
+    if (GV->isDeclaration())
----------------
The comment does not seem to help here, especially since the code has further conditions on the form of the output.


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1620
+
+  llvm_unreachable("Unknown linkage type!");
+}
----------------
This unreachable is "physically" unreachable...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75866