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

[Jason Liu via Phabricator via llvm-commits]

jasonliu added inline comments.


================
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
----------------
jasonliu wrote:
> 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.
Also, we should note explicitly, .comm directory's visibility is not implemented?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75866