This is an archive of the discontinued LLVM Phabricator instance.

[AIX] supporting the visibility attribute for aix assembly
ClosedPublic

Authored by DiggerLin on Mar 9 2020, 12:02 PM.

Details

Summary
  1. in the aix assembly , it do not have .hidden and .protected directive.
  2. in current llvm. if a function or a variable which has visibility attribute, it will generate something like the .hidden or .protected , it can not recognize by aix as.
  3. in aix assembly, the visibility attribute are support in the pseudo-op like

.extern Name [ , Visibility ]
.globl Name [, Visibility ]
.weak Name [, Visibility ]

in this patch, we implement the visibility attribute for the global variable, function or extern function .

for example.

extern __attribute__ ((visibility ("hidden"))) int
  bar(int* ip);
__attribute__ ((visibility ("hidden"))) int b = 0;
__attribute__ ((visibility ("hidden"))) int
  foo(int* ip){
   return (*ip)++;
}
  1. the visibility of .comm linkage do not support , we will have a separate patch for it.
  2. we have the unsupported cases ("default" and "internal") , we will implement them in a a separate patch for it.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
llvm/include/llvm/MC/MCAsmInfo.h
98

Suggestion:
HasVisibilityOnlyWithLinkage = false

llvm/include/llvm/MC/MCStreamer.h
568

Are all the attributes emitted "with visibility" linkage attributes? If so, maybe EmitXCOFFSymbolLinkageWithVisibility?

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
1536

Given just the meaning associated with the naming of "hasVisibilityDirective", why is calling EmitLinkage the correct alternative?

llvm/lib/MC/MCAsmStreamer.cpp
836

Why the extra space? Also below.

llvm/lib/MC/MCXCOFFStreamer.cpp
63

Whole file:
s/Visibilitily/Visibility/g;

66

Silently incorrect output is not desirable. Uncomment this and ensure that we get a sensible failure.

DiggerLin updated this revision to Diff 249487.Mar 10 2020, 1:31 PM
DiggerLin marked 10 inline comments as done.

address comment

llvm/include/llvm/MC/MCAsmInfo.h
98

change as suggestion

llvm/include/llvm/MC/MCStreamer.h
568

not all attribute emitted with "with visibility" . changed as suggestion.

llvm/lib/MC/MCXCOFFStreamer.cpp
66

OK. I uncomment the EmitSymbolAttribute(Symbol,Visibility);
it will hit report_fatal_error("Not implemented yet.");

This patch does not apply to master, and it does not have dependency listed.
We should rebase this patch on master, or find out the right dependencies so that anyone could apply this patch and test.

Are we forgetting the case of commons? They also have visibility as part of the assembler directive on AIX.

llvm/lib/MC/MCXCOFFStreamer.cpp
63

Not just this file, spelling issue is pervasive. Also check 'Visibiltiy'

llvm/include/llvm/MC/MCAsmInfo.h
96

Suggestion (please copy verbatim):

/// True if this is an XCOFF target that supports visibility attributes as
/// part of .global, .weak, .extern, and .comm. Default is false.
llvm/include/llvm/MC/MCStreamer.h
569

Rename Attribute to Linkage to reflect the intended meaning. Please do so for all instances of this function and its overriders.

571

Add a blank line before this comment block.

llvm/lib/MC/MCAsmStreamer.cpp
815

Why is this okay? The visibility would get silently ignored. This should be an llvm_unreachable and caller should avoid calling this function in such a way that this case is hit.

832

Why no MCSA_Internal case?

llvm/lib/MC/MCStreamer.cpp
1069

s/The //;
Also, add a period at the end of the sentence.

llvm/lib/MC/MCXCOFFStreamer.cpp
66

There should be a comment here that MCSA_Invalid is used to indicate that there is no visibility to emit. There should also be a Doxygen comment in the base interface declaration that indicates this aspect of the function interface.

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1582

This needs a TODO for implementing "exported" versus "unspecified" visibility for AIX.

llvm/test/CodeGen/PowerPC/aix-xcoff-hidden.ll
45

There is no testing for referencing the address of a function with hidden visibility that is neither defined nor called directly.

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1579

Linkage and "Visibiltiy" are used only once. I would suggest that we don't need these variables.

1584

There should be a limited number of VisibilityTypes, make this a covering switch.

1595

Why is it okay to ignore visibility when encountering this case?

1598

The comment does not seem to help here, especially since the code has further conditions on the form of the output.

1612

This unreachable is "physically" unreachable...

llvm/lib/MC/MCAsmStreamer.cpp
844

This return is redundant with reaching the end of the function.

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1599

Would it make sense to do
GV->isDeclaration() ? MCSA_Extern : MCSA_Global
?

1607

For AIX asm, I would like to believe that this is always true.

llvm/lib/MC/MCAsmStreamer.cpp
825

I am somewhat surprised to encounter this block of code as being something new in the codebase since the problem it solves should not be a "new problem".

If I understand this correctly, this block of code solves a problem that this patch does need a solution for but is not the subject of this patch. In particular, we are still (without this patch) not emitting the linkage on the assembly path for references to functions that are not defined in the current translation unit. I would suggest splitting this out into a separate patch first. @jasonliu @daltenty, fya.

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1595

Also, please use the immediate base class when explicitly invoking a base implementation of a virtual function.

DiggerLin updated this revision to Diff 249745.Mar 11 2020, 1:53 PM
DiggerLin marked 18 inline comments as done.

address comment

llvm/lib/MC/MCAsmStreamer.cpp
832

not sure when the llvm will emit a internal visibility.

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1584

yes, there are only three visibility

/// An enumeration for the kinds of visibility of global values.
enum VisibilityTypes {
  DefaultVisibility = 0,  ///< The GV is visible
  HiddenVisibility,       ///< The GV is hidden
  ProtectedVisibility     ///< The GV is protected
};
1595

in the patach, we only deal with the GlobalValue::ExternalLinkage
for other linkage , we do not deal with in the patch.

case GlobalValue::CommonLinkage:
case GlobalValue::LinkOnceAnyLinkage:
case GlobalValue::LinkOnceODRLinkage:
case GlobalValue::WeakAnyLinkage:
case GlobalValue::WeakODRLinkage:

we do not deal with this moment, we call the  AsmPrinter::emitLinkage

we maybe need to deal above linkage later in other patch.

1599

if the GV only is declare. we will emit .extern Name [ , Visibility ] here
if the GV is define . we will emit .globl Name [, Visibility ]

jasonliu added inline comments.Mar 11 2020, 2:05 PM
llvm/lib/MC/MCAsmStreamer.cpp
825

Yes, we do not emit .extern for undefined function. And that was fine because we used -u option with assembler to work around that.
And when I look into this change here, I realized that we are changing the architect of how we are emitting the assembly and it definitely deserve a separate patch and we should discuss before we proceed.
Before, we would generate:
.globl foo
with this patch:
.globl foo[DS]

This means we are changing what we are seeing in the symbol table.
Before, we would have 1 csect named foo with internal linkage, and 1 label named foo with external linkage.
Now, we have 1 csect named foo with external inkage. And that's it. But we still emit the useless foo label in the assembly though.
So IMO, there are some clean up we might want to do before we proceed with this ".globl foo[DS]" route, those clean up including: start generating .globl for function descriptor csect, and stop generating the useless function descriptor label, generate .extern for functions and variables, get rid of '-u' option. There are test cases to clean up as well, all those tests checking ".globl foo", we need to change them to check ".globl foo[DS]". Those tests could still be passing now because they are still "match", but we should check what we actually want to test.

llvm/lib/MC/MCAsmStreamer.cpp
825

Emitting .weak (when that is needed) is necessary and fits with emitting .extern.

llvm/include/llvm/MC/MCXCOFFStreamer.h
29

Minor nit: Please declare this in the same order as the base class in relation to the next declaration.

llvm/lib/MC/MCAsmInfoXCOFF.cpp
17

Minor nit: Move this to right after IsLittleEndian to match the ordering in MCAsmInfo.

llvm/lib/MC/MCAsmStreamer.cpp
808

The calls to a function with this name never pass MCSA_Weak. The case makes sense to have but we need some way to get here.

815

I think the message should just say "Unhandled linkage type".

834

This should be the case for MCSA_Invalid, but for other values, we should call llvm_unreachable indicating that we got an unexpected value for Visibility.

llvm/lib/MC/MCStreamer.cpp
1069

There is still no period at the end of the sentence.

llvm/lib/MC/MCXCOFFStreamer.cpp
67

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.
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1581

Fix the indentation. Also, the TODO for the "exported" versus "unspecified" needs to go here.

1583

Be consistent about the use of blank lines between cases.

1595

This is not fixed. Please edit the call to AsmPrinter::emitLinkage.

1599

Which is exactly what using the conditional expression would do also.

1602

Please add spaces:

// TODO: Need to [ ... ]

Please add a comma before "etc.".

CommonLinkage is listed twice?

1603

This should not just ignore the visibility. report_fatal_error if we encounter this case and the VisibilityAttr is not MCSA_Invalid.

llvm/test/CodeGen/PowerPC/aix-xcoff-hidden.ll
45

Please add the requested test.

We(David, Hubert, Jason, Sean and me) has discussed about the visibility this afternoon.

  1. We agree to separate into three patch.

First patch:

cat > test.c
void foo() {};

Currently, llc will generate assembly code as (assembly patch)

.globl  foo
.globl  .foo
.csect foo[DS]

foo:

     .long   .foo
     .long   TOC[TC0]
     .long   0
and symbol table as (xcoff object file)
[4]     m   0x00000004     .data     1  unamex                    foo
[5]     a4  0x0000000c       0    0     SD       DS    0    0
[6]     m   0x00000004     .data     1  extern                    foo
[7]     a4  0x00000004       0    0     LD       DS    0    0
After first patch, the assembly will be as
     .globl  foo[DS]                 # -- Begin function foo
     .globl  .foo
     .align  2
     .csect foo[DS]
     .long   .foo
     .long   TOC[TC0]
     .long   0
 and symbol table will as
[6]     m   0x00000004     .data     1  extern                    foo
[7]     a4  0x00000004       0    0     DS      DS    0    0

we need to change the code for the assembly path and xcoff object file patch for llc.
and also need to modify all the aix test cases which relate to above change
Second Patch.

We will support .extern and .weak directive in Second Patch.

Third Patch

We will support visibility in third patch. (this patch will be the third patch)
DiggerLin edited the summary of this revision. (Show Details)May 6 2020, 6:51 AM
DiggerLin marked 10 inline comments as done.May 6 2020, 9:33 AM
DiggerLin added inline comments.
llvm/lib/MC/MCAsmStreamer.cpp
834

I do not think the MCSA_Invalid will come to here. if come to the function. it should be llvm_unreachable too.

llvm/lib/MC/MCAsmStreamer.cpp
834

PPCAIXAsmPrinter::emitLinkage uses MCSA_Invalid for GlobalValue::DefaultVisibility.

DiggerLin updated this revision to Diff 262418.May 6 2020, 10:40 AM
DiggerLin marked an inline comment as done.

change code based on the emit .extern and .weak patch

jasonliu added inline comments.May 21 2020, 8:40 AM
llvm/include/llvm/MC/MCSymbolXCOFF.h
64

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.

llvm/lib/MC/MCAsmStreamer.cpp
826

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.

836

We don't like spaces between commas: https://reviews.llvm.org/D80247

llvm/test/CodeGen/PowerPC/aix-xcoff-hidden.ll
1

Missing mcpu settings (the default mcpu is not correct for AIX).

56

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.

DiggerLin marked 11 inline comments as done.May 26 2020, 7:03 AM
DiggerLin added inline comments.
llvm/include/llvm/MC/MCSymbolXCOFF.h
64

thanks ,changed to XCOFF::SYM_V_UNSPECIFIED

64

thanks for your suggestion.

llvm/test/CodeGen/PowerPC/aix-xcoff-hidden.ll
1

thanks

56

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"

56

added a protected test case

DiggerLin updated this revision to Diff 266213.May 26 2020, 8:06 AM
DiggerLin marked 3 inline comments as done.

address comment

jasonliu added inline comments.May 27 2020, 1:11 PM
llvm/include/llvm/BinaryFormat/XCOFF.h
162

I feel like "Symbol" here is redundant. We could just name it VisibilityType, No?

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
1521
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.

llvm/lib/MC/MCAsmStreamer.cpp
815

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.

816

nit: do we need this return?

819

Remove unused local variable: XCOFFSym

825

Should be report_fatal_error?

llvm/lib/MC/MCXCOFFStreamer.cpp
67

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?

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1578

Do we want to assert here?
assert(MAI->HasVisibilityOnlyWithLinkage(), "AIX's linkage directive have visibility setting.");
which serves as a documentation as well.

1582

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.

1583

nit: remove extra blank line?

1609

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.

llvm/test/CodeGen/PowerPC/aix-xcoff-hidden.ll
10

We are testing visibility here, do you really need all this code block inside "foo", "foo_h", "foo_protected", "foo_weak_h"?

56

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.

jasonliu added inline comments.May 27 2020, 1:15 PM
llvm/test/CodeGen/PowerPC/aix-xcoff-hidden.ll
56

Also, we should note explicitly, .comm directory's visibility is not implemented?

jasonliu added inline comments.May 27 2020, 1:23 PM
llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
1521

Sorry, I think this might be better.

if (TM.getTargetTriple().isOSBinFormatXCOFF())
  if (!F.isIntrinsic())
    continue;
  ...
  emitLinkage()
} else {
  ...
  emitVisibility()
}
DiggerLin marked 20 inline comments as done.May 29 2020, 1:30 PM
DiggerLin added inline comments.May 29 2020, 1:30 PM
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1609

I added AvailableExternallyLinkage . I think we have deal with all the linkage with visibility. others we just pass to the AsmPrinter::emitLinkage(GV, GVSym) and let AsmPrinter::emitLinkage deal with it.

DiggerLin marked 2 inline comments as done.May 29 2020, 1:31 PM
DiggerLin added inline comments.
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1609

sorry, please discard above comment.

DiggerLin updated this revision to Diff 267360.May 29 2020, 1:48 PM

address comment

jasonliu added inline comments.Jun 1 2020, 8:31 AM
llvm/include/llvm/MC/MCSymbolXCOFF.h
58

const before { ?

llvm/lib/MC/MCAsmStreamer.cpp
33

how many of these newly added include directive are actually needed now?

802

Could we add const for the Symbol argument?

815

Should we do a report_fatal_error instead?

llvm/lib/MC/MCXCOFFStreamer.cpp
67

I don't think removing

if (Visibility == MCSA_Invalid)
    return;

before emit symbol attribute for visibility, and skip the requested comment is a good way to move forward.
Without those, there is no clear contract to tell people how to use this function.

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1578

This is marked done, but I'm not seeing anything?

1582

'versus' does not sound right in the current context. It should be 'and'?

1585

We should call the direct derived class's method (even when that class did not override this function).

1597

I think it might be better to do assignments here just like the visibility above. And do only one call out to emitXCOFFSymbolLinkageWithVisibility outside of this switch statement. It would help the readability of this code block as it clearly showed that this switch only modify one argument.

1613

nit: commonLinkage -> CommonLinkage
The name is spelled with Capitalization, so we should stick with that.

llvm/test/CodeGen/PowerPC/aix-xcoff-hidden.ll
2

nit: a split for these line would be helpful? As it's a tad too long.

56

I'm not seeing this getting responded or addressed.

DiggerLin edited the summary of this revision. (Show Details)Jun 2 2020, 12:51 PM
DiggerLin marked 14 inline comments as done.Jun 2 2020, 1:42 PM
DiggerLin added inline comments.
llvm/lib/MC/MCAsmStreamer.cpp
33

thanks

802

sorry , I can not add const in this patch, for in the function
MCXCOFFStreamer::emitXCOFFSymbolLinkageWithVisibility()

the function calls the
bool MCXCOFFStreamer::emitSymbolAttribute(MCSymbol *Sym,Symbol, Visibility)

. if we want to change to " const MCSymbol *Sym" , we can add a NFC patch later.

llvm/lib/MC/MCXCOFFStreamer.cpp
67

if Visibility is MCSA_Invalid, it will not come to function emitXCOFFSymbolLinkageWithVisibility

DiggerLin marked 4 inline comments as done.Jun 2 2020, 1:43 PM
DiggerLin added inline comments.
llvm/test/CodeGen/PowerPC/aix-xcoff-hidden.ll
56

added into the patch's summary

DiggerLin updated this revision to Diff 267989.Jun 2 2020, 2:01 PM

address comment

jasonliu added inline comments.Jun 2 2020, 2:30 PM
llvm/lib/MC/MCAsmStreamer.cpp
802

You are right. I didn't realize it needs to be non-const for MCXCOFFStreamer version.

jasonliu added inline comments.Jun 3 2020, 10:14 AM
llvm/lib/MC/MCXCOFFStreamer.cpp
67

Thanks. I would actually want the emitXCOFFSymbolLinkageWithVisibility to handle MCSA_Invalid case. Please see my other comment.

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1587

I have some concern about this call back to the original AsmPrinter::emitLinkage.
It seems this is just a short cut so that we don't need to handle the linkage emission with unspecified visibility.
But I would argue that taking this short cut might not worth it, as it's not too much work for the emitXCOFFSymbolLinkageWithVisibility to handle the unspecified visibility case.
If we don't need to call back to the base AsmPrinter::emitLinkage, then we could remove a lot of XCOFF-related staff out from AsmPrinter::emitLinkage which would make the code a lot cleaner. Also we could avoid dual-maintaining two emitLinkage method for AIX.

DiggerLin marked 4 inline comments as done.Jun 3 2020, 1:56 PM
DiggerLin added inline comments.
llvm/lib/MC/MCXCOFFStreamer.cpp
67

please see my other comment.

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1587

some linkage do not have visibility.
for example
InternalLinkage .lglobl , it should go to PPCAsmPrinter::emiltLinkage.

and there also have PrivateLinkage and AppendingLinkage, we do not deal with in the function , it just need to pass to function PPCAsmPrinter::emiltLinkage to deal with it.

I discuss with jason offline, Jason's option is "we should deal with all the linkage and visibility in current function without invoke the PPCAsmPrinter::emiltLinkage()" .
@hubert.reinterpretcast , what is your option ?

DiggerLin updated this revision to Diff 268485.Jun 4 2020, 8:20 AM
DiggerLin marked 2 inline comments as done.

address comment

jasonliu accepted this revision.Jun 4 2020, 10:20 AM

LGTM with minor nit.

llvm/include/llvm/MC/MCStreamer.h
572

nit, we need to mention it here as Doxygen style:
When the symbol do not have any visibility setting, pass in MCSA_Invalid.

llvm/lib/MC/MCAsmStreamer.cpp
821

nit: we should make the default case position more consistent at least within this function/file.

822

nit: Visibility -> visibility type

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1587

Thanks for the change. I do like what we have right now because we don't need to worry if the base emitLinkage have the correct implementation for AIX or not.
Are we planning to clean up the base emitLinkage (remove XCOFF-related changes) in a separate NFC patch?

This revision is now accepted and ready to land.Jun 4 2020, 10:20 AM
llvm/include/llvm/BinaryFormat/XCOFF.h
162

A lot of the comments in the file should be Doxygen-style, but the relative dearth of them is no reason not to use one here.

We should have a comment:
Values for visibility as they would appear when encoded in the high 4 bits of the 16-bit unsigned n_type field of symbol table entries. Valid for 32-bit XCOFF only when the vstamp in the auxiliary header is greater than 1.

llvm/include/llvm/MC/MCStreamer.h
568

s/in linkage directive/with a linkage directive/;

572

Suggestion:
... to emit or MCSA_Invalid is the symbol does not have an explicit visibility.

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
1510

I would suggest flipping this:

if (!TM.getTargetTriple().isOSBinFormatXCOFF()) {
  // ...
  continue;
}

if (F.isIntrinsic())
  continue;

// ...
llvm/lib/MC/MCAsmInfoXCOFF.cpp
17

This should be rebased for new changes.

llvm/lib/MC/MCAsmStreamer.cpp
824

Should there be a comment like

// Nothing to do.

?

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1580

s/directive have visibility/directives take a visibility/;

llvm/test/CodeGen/PowerPC/aix-xcoff-hidden.ll
2

Please fix the two consecutive spaces before -mtriple.

4

Same comment.

10

Fix the two spaces before the closing brace.

15

Fix the two spaces after define.

20

Same comment.

44

This does not actually prevent ,something from being on this line.
Add {{[[:space:]]*([#].*)?$}}.

45

Same comment.

48

The file is called aix-xcoff-hidden.ll? Should it be renamed?

55

Does it make sense to have a blank line between the defined variables and the undefined symbols?

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1600

Should there be an assert for MAI->hasDotLGloblDirective()?

1603
llvm_unreachable("Should never emit this");

is right. This linkage type is an LLVM IR artifact.

1605

I believe that llvm_unreachable is generally correct for cases where the source of the error condition is not from "user input" but instead from API usage error.

1606

Please assert here that LinkageAttr is not still MCSA_Invalid.

DiggerLin marked 27 inline comments as done.Jun 5 2020, 8:01 AM
DiggerLin added inline comments.
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1587

Only one place need to clean in the base emitLinkage, I clean it in this patch.

llvm/test/CodeGen/PowerPC/aix-xcoff-hidden.ll
48

thanks

DiggerLin updated this revision to Diff 268848.Jun 5 2020, 9:35 AM
DiggerLin marked an inline comment as done.

rebase the patch and address comment

jasonliu added inline comments.Jun 5 2020, 10:41 AM
llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
424

hasDotExternDirective is XCOFF only.
Also, please check if we have other usage of this MAI property. If not, we could remove them together.

432

XCOFF only. Clean up needed. Please also check if we could remove this property from MAI.

436

This is XCOFF only, which could get clean up.

440–441

XCOFF only. Clean up needed.

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1587

There is more than one place. Please check my other comments if we want to clean it in this patch.

DiggerLin marked 2 inline comments as done.Jun 6 2020, 1:55 PM

in order not to let this patch too big, I will create NFC patch to clean up the AsmPrinter::emitLinkage.

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
432

hasDotLGloblDirective only use here. in order not to let the patch too big . I will remove hasDotLGloblDirective and DotLGloblDirective in new NFC patch.

DiggerLin marked an inline comment as done.Jun 6 2020, 1:55 PM
DiggerLin updated this revision to Diff 269029.Jun 6 2020, 2:04 PM

address comment

DiggerLin marked 3 inline comments as done.Jun 6 2020, 2:05 PM
llvm/include/llvm/MC/MCStreamer.h
568

Still missing "a" before linkage.

573

There was a typo in my suggestion:
s/is/if/;

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
1528

Suggestion:

// Handle the XCOFF case.
// `Name` is the function descriptor symbol (see above). Get the function
// entry point symbol.
llvm/lib/MC/MCAsmInfoXCOFF.cpp
17

Move this to right after IsLittleEndian to maintain matching the base class order.

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1600

The string should be "Expecting .lglobl to be supported for AIX."

llvm/test/CodeGen/PowerPC/aix-xcoff-visibility.ll
52 ↗(On Diff #269029)

Good catch; thanks.

DiggerLin updated this revision to Diff 269331.Jun 8 2020, 1:06 PM
DiggerLin marked 5 inline comments as done.

address comment

LGTM with small comment.

llvm/lib/MC/MCAsmInfoXCOFF.cpp
17

? I said to move HasVisibilityOnlyWithLinkage up, not IsLittleEndian down.

IsLittleEndian = false;
HasVisibilityOnlyWithLinkage = true;
PrivateGlobalPrefix = "L..";
PrivateLabelPrefix = "L..";
This revision was automatically updated to reflect the committed changes.
DiggerLin marked an inline comment as done.