[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/BinaryFormat/XCOFF.h:162
 
+enum SymbolVisibilityType : uint16_t {
+  SYM_V_UNSPECIFIED = 0x0000,
----------------
I feel like "Symbol" here is redundant. We could just name it VisibilityType, No?


================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:1521
       emitLinkage(&F, Name);
+      continue;
     }
----------------
```
if (TM.getTargetTriple().isOSBinFormatXCOFF() && !F.isIntrinsic()) {
  ...
  emitLinkage()
} else {
  ...
  emitVisibility()
}
```

Might be a better choice than "continue" here. If F is an intrinsic then it would go to "emitVisibility" block even if it's XCOFF target. It may or may not do any harm, but I think it would be better not go to there.



================
Comment at: llvm/lib/MC/MCAsmStreamer.cpp:816
+    llvm_unreachable("Visibility is not a part of this linkage type.");
+    return;
+  }
----------------
nit: do we need this `return`?


================
Comment at: llvm/lib/MC/MCAsmStreamer.cpp:819
+
+  MCSymbolXCOFF *XCOFFSym = cast<MCSymbolXCOFF>(Symbol);
+
----------------
Remove unused local variable: XCOFFSym


================
Comment at: llvm/lib/MC/MCAsmStreamer.cpp:825
+  default:
+    llvm_unreachable("unexpected value for Visibility.");
+  case MCSA_Hidden:
----------------
Should be report_fatal_error?


================
Comment at: llvm/lib/MC/MCAsmStreamer.cpp:814
+  default:
+    llvm_unreachable("Visibility is not a part of this linkage type.");
+    return;
----------------
hubert.reinterpretcast wrote:
> I think the message should just say "Unhandled linkage type".
> I think the message should just say "Unhandled linkage type".
This comment is not addressed.

Should this really be llvm_unreachable? It's possible someone passed in a wrong value by mistake, and we would still like to catch that in a non-assert build. Also I don't think we would have a good binary if we hit this. A report_fatal_error might be preferable.


================
Comment at: llvm/lib/MC/MCXCOFFStreamer.cpp:57
+
+  ///< Not emit default visibility.
+  if (Visibility == MCSA_Invalid)
----------------
hubert.reinterpretcast wrote:
> The comment here should not be Doxygen style. There should be a comment elsewhere (in the header for the base class declaration) that //is// in Doxygen style. Also, "default visibility" is ambiguous for XCOFF.
> 
> Suggestion:
> ```
>   // When the caller passes `MCSA_Invalid` for the visibility, do not emit one.
> ```
> There should be a comment elsewhere (in the header for the base class declaration) that is in Doxygen style.
I'm not seeing this comment being addressed?


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1578
+                                   MCSymbol *GVSym) const {
+
+  MCSymbolAttr VisibilityAttr = MCSA_Invalid;
----------------
Do we want to assert here?
assert(MAI->HasVisibilityOnlyWithLinkage(), "AIX's linkage directive have visibility setting.");
which serves as a documentation as well. 


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1582
+
+    // TODO: "exported" versus "unspecified" Visibility needs to go here.
+
----------------
Right now, we already implemented "unspecified" visibility using GlobalValue::DefaultVisibility. So the comment is not correct.
I think we are missing "exported" and "internal" visibility.


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1583
+    // TODO: "exported" versus "unspecified" Visibility needs to go here.
+
+  case GlobalValue::DefaultVisibility:
----------------
nit: remove extra blank line?


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1609
+  default:
+    // TODO: Need to deal with other Linkages etc.
+    AsmPrinter::emitLinkage(GV, GVSym);
----------------
What is the extra linkage we need to deal here?
If we know the extra linkage we need to deal with and we don't want to deal with them now, then we have separate cases for those extra linkages and call `AsmPrinter::emitLinkage(GV, GVSym);`.
Ideally, I would want the default case to be `report_fatal_error` so that if we miss handling something, it would trigger.


================
Comment at: llvm/test/CodeGen/PowerPC/aix-xcoff-hidden.ll:9
+entry:
+  %ip.addr = alloca i32*, align 4
+  store i32* %ip, i32** %ip.addr, align 4
----------------
We are testing visibility here, do you really need all this code block inside "foo", "foo_h", "foo_protected", "foo_weak_h"? 


================
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
----------------
DiggerLin wrote:
> DiggerLin wrote:
> > jasonliu wrote:
> > > 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. 
> > I tried the 
> > ```
> > __attribute__ ((visibility ("default"))) void foo_default(int *p){
> >   (*p)++;
> > }
> > 
> > __attribute__ ((visibility ("internal"))) void foo_internal(int *p){
> >   (*p)++;
> > } 
> > 
> > with clang it convert to  
> > define void @foo_default(i32* %p) #0 {
> > entry:
> >   %p.addr = alloca i32*, align 4
> >   store i32* %p, i32** %p.addr, align 4
> >   %0 = load i32*, i32** %p.addr, align 4
> >   %1 = load i32, i32* %0, align 4
> >   %inc = add nsw i32 %1, 1
> >   store i32 %inc, i32* %0, align 4
> >   ret void
> > }
> > 
> > define hidden void @foo_internal(i32* %p) #0 {
> > entry:
> >   %p.addr = alloca i32*, align 4
> >   store i32* %p, i32** %p.addr, align 4
> >   %0 = load i32*, i32** %p.addr, align 4
> >   %1 = load i32, i32* %0, align 4
> >   %inc = add nsw i32 %1, 1
> >   store i32 %inc, i32* %0, align 4
> >   ret void
> > }
> > ```
> > clang look "default" visibility as no visibility, and look "internal" as "hidden"
> > ```
> >  static Visibility parseVisibility(Arg *arg, ArgList &args,
> >                                   DiagnosticsEngine &diags) {
> >   StringRef value = arg->getValue();
> >   if (value == "default") {
> >     return DefaultVisibility;
> >   } else if (value == "hidden" || value == "internal") {
> >     return HiddenVisibility;
> >   } else if (value == "protected") {
> >     // FIXME: diagnose if target does not support protected visibility
> >     return ProtectedVisibility;
> >   }
> > ```
> >   diags.Report(diag::err_drv_invalid_value)
> >     << arg->getAsString(args) << value;
> >   return DefaultVisibility;
> > }
> > 
> > I think we need a separate patch for "default" and "hidden"
> added a protected test case
Could we have the unsupported cases ("default" and "internal") documented in the patch summary(later in commit message)? 
We should say this patch only implements visibility "unspecifeid", "protected" and "hidden" on AIX. "default" and "internal" will be handled in a separate patch because they would require IR level change in clang to support.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75866