[PATCH] D75866: [AIX] supporting the visibility attribute for aix assembly
Jason Liu via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Jun 1 08:34:00 PDT 2020
jasonliu added inline comments.
================
Comment at: llvm/include/llvm/MC/MCSymbolXCOFF.h:58
+
+ XCOFF::VisibilityType getVisibilityType() { return VisibilityType; }
+
----------------
`const` before `{` ?
================
Comment at: llvm/lib/MC/MCAsmStreamer.cpp:814
+ default:
+ llvm_unreachable("Visibility is not a part of this linkage type.");
+ return;
----------------
jasonliu wrote:
> 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.
Should we do a `report_fatal_error` instead?
================
Comment at: llvm/lib/MC/MCAsmStreamer.cpp:33
#include "llvm/MC/MCStreamer.h"
+#include "llvm/MC/MCSymbolXCOFF.h"
#include "llvm/Support/ErrorHandling.h"
----------------
how many of these newly added include directive are actually needed now?
================
Comment at: llvm/lib/MC/MCAsmStreamer.cpp:802
+void MCAsmStreamer::emitXCOFFSymbolLinkageWithVisibility(
+ MCSymbol *Symbol, MCSymbolAttr Linkage, MCSymbolAttr Visibility) {
+
----------------
Could we add `const` for the `Symbol` argument?
================
Comment at: llvm/lib/MC/MCXCOFFStreamer.cpp:57
+
+ ///< Not emit default visibility.
+ if (Visibility == MCSA_Invalid)
----------------
jasonliu wrote:
> 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?
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.
================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1578
+ MCSymbol *GVSym) const {
+
+ MCSymbolAttr VisibilityAttr = MCSA_Invalid;
----------------
jasonliu wrote:
> Do we want to assert here?
> assert(MAI->HasVisibilityOnlyWithLinkage(), "AIX's linkage directive have visibility setting.");
> which serves as a documentation as well.
This is marked done, but I'm not seeing anything?
================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1582
+
+ // TODO: "exported" versus "internal" Visibility needs to go here.
+
----------------
'versus' does not sound right in the current context. It should be 'and'?
================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1585
+ case GlobalValue::DefaultVisibility:
+ AsmPrinter::emitLinkage(GV, GVSym);
+ return;
----------------
We should call the direct derived class's method (even when that class did not override this function).
================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1597
+ case GlobalValue::ExternalLinkage:
+ OutStreamer->emitXCOFFSymbolLinkageWithVisibility(
+ GVSym, GV->isDeclaration() ? MCSA_Extern : MCSA_Global, VisibilityAttr);
----------------
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.
================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1613
+ case GlobalValue::CommonLinkage:
+ report_fatal_error("commonLinkage of XCOFF should not come to this path");
+ default:
----------------
nit: commonLinkage -> CommonLinkage
The name is spelled with Capitalization, so we should stick with that.
================
Comment at: llvm/test/CodeGen/PowerPC/aix-xcoff-hidden.ll:1
+; RUN: llc -verify-machineinstrs -mtriple powerpc-ibm-aix-xcoff -mcpu=pwr4 -mattr=-altivec < %s | FileCheck %s
+; RUN: llc -verify-machineinstrs -mtriple powerpc64-ibm-aix-xcoff -mcpu=pwr4 -mattr=-altivec < %s | FileCheck %s
----------------
nit: a split for these line would be helpful? As it's a tad too long.
================
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:
> 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?
I'm not seeing this getting responded or addressed.
Repository:
rZORG LLVM Github Zorg
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D75866/new/
https://reviews.llvm.org/D75866
More information about the llvm-commits
mailing list