[PATCH] D70724: [PowerPC] Add Support for indirect calls on AIX.

[Hubert Tong via Phabricator via llvm-commits]

hubert.reinterpretcast added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:7052
+    const unsigned TOCSaveOffset =
+                   Subtarget.getFrameLowering()->getTOCSaveOffset();
+
----------------
Nit: Indentation does not match `clang-format`:
```
    const unsigned TOCSaveOffset =
        Subtarget.getFrameLowering()->getTOCSaveOffset();
```


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:7061
+                         MachinePointerInfo::getStack(
+                                DAG.getMachineFunction(), TOCSaveOffset));
+  }
----------------
Nit: Indentation does not match `clang-format`:
```
    Chain = DAG.getStore(
        Val.getValue(1), dl, Val, AddPtr,
        MachinePointerInfo::getStack(DAG.getMachineFunction(), TOCSaveOffset));
```


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrFormats.td:1532
 
+class XLForm_2_ext_and_DForm_1<bits<6> opcode1, bits<10>xo1, bits<5> bo,
+                               bits<5> bi, bits<2> bh, bit lk, bits<6> opcode2,
----------------
Nit: Missing space before `xo1`.


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrFormats.td:1533
+class XLForm_2_ext_and_DForm_1<bits<6> opcode1, bits<10>xo1, bits<5> bo,
+                               bits<5> bi, bits<2> bh, bit lk, bits<6> opcode2,
+                               dag OOL, dag IOL, string asmstr,
----------------
Please refer to `XLForm_2_ext`. There should be no parameter taken for `bh`.


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrFormats.td:1539
+  bits<5>  RST;
+  bits<19> DS_RA;
+
----------------
Refer to `DForm_1`. This should be `bits<21> Addr`.


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrFormats.td:1545
+  let Inst{11-15} = bi;
+  let Inst{16-18} = 0;  // unused.
+  let Inst{19-20} = bh;
----------------
Nit: This comment is a label, not a sentence. Do not use a period, but do capitalize.


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrFormats.td:1552
+  let Inst{43-47} = DS_RA{18-14};  // Register #
+  let Inst{48-61} = DS_RA{13-0};   // Displacement.
+}
----------------
Same nit about labels versus sentences.


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrFormats.td:1552
+  let Inst{43-47} = DS_RA{18-14};  // Register #
+  let Inst{48-61} = DS_RA{13-0};   // Displacement.
+}
----------------
hubert.reinterpretcast wrote:
> Same nit about labels versus sentences.
This is incorrect for D-form. The D field has 16 bits.


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.td:1652
+    XLForm_2_ext_and_DForm_1<19, 528, 20, 0, 0, 1, 32, (outs),
+     (ins memrix:$src), "bctrl\n\tlwz 2, $src", IIC_BrB,
+     [(PPCbctrl_load_toc iaddrX4:$src)]>, Requires<[In32BitMode]>;
----------------
Should this be `memri` (accepting non-aligned operands)?


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.td:1653
+     (ins memrix:$src), "bctrl\n\tlwz 2, $src", IIC_BrB,
+     [(PPCbctrl_load_toc iaddrX4:$src)]>, Requires<[In32BitMode]>;
+
----------------
Should this be `iaddr` (i.e., D-form)?


================
Comment at: llvm/test/CodeGen/PowerPC/aix_indirect_call.ll:145
+; OBJ32-LABEL: .callThroughPtrWithArgs:
+; OBJ32:         stwu 1, -64(1)
+; OBJ32-DAG:     lwz [[REG:[0-9]+]], 0(3)
----------------
Seems like it might be less duplication if the grouping was `--check-prefixes=ASMOBJ32,ASM32` and `--check-prefixes=ASMOBJ32,OBJ32`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70724