[PATCH] D66724: [AIX]Emit function descriptor csect in assembly

[Sean Fertile via Phabricator via llvm-commits]

sfertile added inline comments.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:1657
   // Get the function symbol.
-  CurrentFnSym = getSymbol(&MF.getFunction());
+  if (MAI->needsFunctionDescriptors()) {
+    assert(TM.getTargetTriple().isOSAIX() && "Function descriptor is only"
----------------
The code for creating the entry point and descriptor symbols and their csects looks good, I don't think it needs any changes for functionality/correctness. I have a suggestion related to style though. We are inserting about 20 lines of AIX specific implementation details into a constructor thats only about 15 lines long before the change. Most developers that read this won't care about the AIX specific details, should we split the AIX entry point and descriptor setup into some helper functions? Something like

```
if ((MAI->needsFunctionDescriptors()) {
   assert(TM.getTargetTriple().isOSAIX() && "Function descriptor is only"
                                             " supported on AIX.");
  CurrentFnDescSym = getAIXDescriptorSymbol(MF.getFunction(), OutStreamer->getContext());
  CurrentFnSym = getAIXEntryPointSymbol(MF.getFunction(),  cast<MCSectionXCOFF>(getObjFileLowering(), TM);
} else {
```

keeps this function very readable by having descriptive helper functions bury the implementation details.


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1731
+void PPCAIXAsmPrinter::EmitEndOfAsmFile(Module &M) {
+  // If there are no functions in this module, we will never need to reference
+  // the TOC anchor.
----------------
There may be symbol references we could emit in assembly that require a TOC-base. Its not really an issue now but it may be once we implement the integrated assembler for AIX. I'm not sure if its better to be `sloppy` and simply always emit a TOC-base (an unused one would be benign ...) 


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1733
+  // the TOC anchor.
+  if (M.size() > 0) {
+    StringRef TOC("TOC");
----------------
```
if (M.empty())
  return;
```

Prefer an early return to nesting all the code inside an if statement.


================
Comment at: llvm/test/CodeGen/PowerPC/test_func_desc.ll:15
+entry:
+  %retval = alloca i32, align 4
+  %a = alloca i32, align 4
----------------
This IR is overly verbose, could we replace it with 

```
  %1 = call i32 @foo()
  %2 = call i32 @foo_extern()
  %3 = call i32 @foo_static()
  %4 = add nsw i32 %1, %2
  %5 = add nsw i32 %4, %3
  ret i32 %5
```

(If I'm missing something from the test, you can generate less verbose IR by feeding this through `opt -S -mem2reg`)


================
Comment at: llvm/test/CodeGen/PowerPC/test_func_desc.ll:59
+; CHECK: .csect main[DS]
+; CHECK-NEXT: main
+; 32BIT: .long .main
----------------
should this be `main:`? I believe we are checking for label for the symbol `main`.


================
Comment at: llvm/test/CodeGen/PowerPC/test_func_desc.ll:74
+; CHECK: .csect static_foo[DS]
+; CHECK-NEXT: static_foo
+; 32BIT: .long .static_foo
----------------
ditto. (`static_foo:`)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66724