[PATCH] D62532: [AIX] Implement function descriptor on SDAG

[Hubert Tong via Phabricator via llvm-commits]

hubert.reinterpretcast added a comment.

The commit message should explain something about function descriptor symbols on AIX and how they differ from the symbol that is created for the function entry point.



================
Comment at: llvm/lib/CodeGen/LLVMTargetMachine.cpp:207
+      const_cast<TargetLoweringObjectFile&>(*this->getObjFileLowering())
+        .Initialize(Ctx, *this);
+    }
----------------
Use `clang-format` (pay attention to the spaces):
```
      const_cast<TargetLoweringObjectFile &>(*this->getObjFileLowering())
          .Initialize(Ctx, *this);
```



================
Comment at: llvm/lib/CodeGen/SelectionDAG/FastISel.cpp:1878
   case Instruction::Call:
+    // On AIX, call lowering should always use the DAG-ISEL path. The callee of
+    // the call instruction has to be mapped to the symbol for the functions
----------------
Is this statement of fundamental (permanent) fact? Please make the comment clear on that point. Clarify the relation between the first and the second sentences. For example, does the second sentence indicate something that is only done if we return `false` for AIX?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/FastISel.cpp:1879
+    // On AIX, call lowering should always use the DAG-ISEL path. The callee of
+    // the call instruction has to be mapped to the symbol for the functions
+    // entry point, which is distinct from the function descriptor symbol with
----------------
s/functions/function's/;


================
Comment at: llvm/lib/CodeGen/SelectionDAG/FastISel.cpp:1880
+    // the call instruction has to be mapped to the symbol for the functions
+    // entry point, which is distinct from the function descriptor symbol with
+    // the same C-linkage name as the source level function.
----------------
Split out the last part into a separate sentence:
The latter is the symbol whose XCOFF symbol name is the C-linkage name of the source level function.


================
Comment at: llvm/lib/CodeGen/TargetPassConfig.cpp:203
 
+static cl::opt<bool> InitMC("init-MC", cl::Hidden,
+    cl::desc("Init target object file lowering when in use together with"
----------------
Use `clang-format`:
```
static cl::opt<bool>
    InitMC("init-MC", cl::Hidden,
           cl::desc("Init target object file lowering when in use together "
                    "with -stop-after, -stop-before"));
```

In any case, notice that the current patch has a missing space before `-stop-after` in the string.


================
Comment at: llvm/lib/CodeGen/TargetPassConfig.cpp:493
+
+
 // Helper to verify the analysis is really immutable.
----------------
Remove the extra blank line.


================
Comment at: llvm/lib/Target/PowerPC/PPC.h:91
+    /// to "FOO at plt".  This is used for calls and jumps to external functions 
+    /// and for PIC calls on 32-bit ELF systems.
     MO_PLT = 1,
----------------
This looks like a drive-by fix. If it is not, then clarify the relation of this comment to AIX.


================
Comment at: llvm/lib/Target/PowerPC/PPCFastISel.cpp:1967
     case Instruction::Call:
+      // On AIX, call lowering should always use the DAG-ISEL path. The callee
+      // of the call instruction has to be mapped to the symbol for the 
----------------
See the comments I made on `FastISel.cpp`.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:4416
+    const auto *S = dyn_cast<MCSymbolXCOFF>(M->getMCSymbol());
+    GV = S->getGlobalValue(); 
+  }  
----------------
Using `dyn_cast` and then dereferencing the result without checking for null is not right. If the cast is expected to succeed, use `cast`. In that case, please also add a comment to explain why the cast is expected to succeed.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:4419
+  
+  // If we failed to get a GlobalValue then pessimistically assume they do not
+  // share a TOCBase.
----------------
Add a comma before "then".


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:4918
+    if (TM.getTargetTriple().isOSAIX()) {
+      // Direct function calls reference the symbol for the functions entry 
+      // point, which is named by pre-pending a "." to the functions C-linkage name.
----------------
s/functions/function's/;


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:4919
+      // Direct function calls reference the symbol for the functions entry 
+      // point, which is named by pre-pending a "." to the functions C-linkage name.
+      auto &Context = MF.getMMI().getContext();
----------------
s/pre-pending a "." to/inserting a "." before/;
s/functions/function's/;


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:4923
+                                              Twine(G->getGlobal()->getName()));
+      dyn_cast<MCSymbolXCOFF>(S)->setGlobalValue(GV);
+      Callee = DAG.getMCSymbol(S, PtrVT);
----------------
`dyn_cast` may return a null pointer. Please use `cast` if you expect the cast to succeed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62532