[PATCH] D76932: [AIX] emit .extern and .weak directive linkage

[Jason Liu via Phabricator via cfe-commits]

jasonliu added inline comments.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:1486
 
   // Emit visibility info for declarations
   for (const Function &F : M) {
----------------
Comment should change to something similar to:
`Emit linkage(XCOFF) and visibility info for declarations.`


================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:1510
+      continue;
+    }
+
----------------
What if we have
```
void foo();
void (*foo_ptr)() = foo;
int bar() {
  foo();
}
```
We would need both `.extern .foo` and `.extern foo[DS]`.
Also, please have a similar case into the lit test. 


================
Comment at: llvm/lib/MC/MCXCOFFStreamer.cpp:38
   case MCSA_Global:
+  case llvm::MCSA_Extern:
     Symbol->setStorageClass(XCOFF::C_EXT);
----------------
Please remove `llvm::` for MCSA_Extern and MCSA_Weak to make the style consistent. 


================
Comment at: llvm/lib/MC/MCXCOFFStreamer.cpp:48
+    Symbol->setStorageClass(XCOFF::C_WEAKEXT);
+    Symbol->setExternal(true);
+    break;
----------------
Maybe we should just move `Symbol->setExternal(true);` outside of the switch, as it is set for every attribute that we are going to emit. 


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:351
+      if (nameShouldBeInStringTable(ContainingCsect->getSectionName()))
+        Strings.add(ContainingCsect->getSectionName());
+    }
----------------
We should `continue` here if the rest of the logic does not matter. 


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:352
+        Strings.add(ContainingCsect->getSectionName());
+    }
       // If the symbol is the csect itself, we don't need to put the symbol
----------------
A new line after '}'.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:353
+    }
       // If the symbol is the csect itself, we don't need to put the symbol
       // into csect's Syms.
----------------
line 353 to 365 did not align properly. 


================
Comment at: llvm/test/CodeGen/PowerPC/aix-extern-weak.ll:17
+
+define weak void @foo_weak() #0 {
+entry:
----------------
Nit: Please remove #0, #1 from the test case.


================
Comment at: llvm/test/CodeGen/PowerPC/aix-extern-weak.ll:55
+
+; COMMON:  	.weak	foo_weak[DS]            # -- Begin function foo_weak
+; COMMON-NEXT:	.weak	.foo_weak
----------------
A proper space alignment would make the expected result more readable. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76932