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

[Digger via Phabricator via llvm-commits]

DiggerLin marked 11 inline comments as done.
DiggerLin added inline comments.


================
Comment at: llvm/include/llvm/MC/MCSymbolXCOFF.h:64
   MCSectionXCOFF *RepresentedCsect = nullptr;
+  XCOFF::SymbolVisibilityType VisibilityType = XCOFF::SYM_V_INTERNAL;
 };
----------------
jasonliu wrote:
> 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.
thanks ,changed to XCOFF::SYM_V_UNSPECIFIED


================
Comment at: llvm/include/llvm/MC/MCSymbolXCOFF.h:64
   MCSectionXCOFF *RepresentedCsect = nullptr;
+  XCOFF::SymbolVisibilityType VisibilityType = XCOFF::SYM_V_INTERNAL;
 };
----------------
DiggerLin wrote:
> jasonliu wrote:
> > 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.
> thanks ,changed to XCOFF::SYM_V_UNSPECIFIED
thanks for your suggestion.


================
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
----------------
jasonliu wrote:
> Missing mcpu settings (the default mcpu is not correct for AIX).
thanks


================
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:
> 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"


================
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:
> 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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75866