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

Tue Jun 4 14:58:55 PDT 2019

hubert.reinterpretcast added inline comments.

================
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
----------------
Xiangling_L wrote:
> jasonliu wrote:
> > hubert.reinterpretcast wrote:
> > > 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?
> > I vaguely remember the discussion being... we will disable this fast-isel path for now on AIX and get the non-fast-isel path working first. We would deal with fast-isel path later down the road. 
> > If my memory is correct, then I don't think it is a "permanent fact". So maybe we don't want to use words like "should always", and state that this would be "for now" and we will evaluate the situation later. 
> If we don't always ban fast-isel of call loweing on AIX, does that mean we need to duplicate logic of creating MCSymbolSDNode/MCSymbolXCOFF for function entry point on fast-isel path?
I agree with Jason. There should be something about how the fast-isel path does not do the mapping at this time, and the "can be mapped" should say "will be mapped" to avoid the connotation that doing the mapping cannot be done other than by going through DAG-ISEL.

================
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 
----------------
hubert.reinterpretcast wrote:
> See the comments I made on `FastISel.cpp`.
Make the same tweaks to the comment here.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62532