[PATCH] D91053: [PowerPC] Lump the constants to save one addis for each constant access

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai requested changes to this revision.
nemanjai added a comment.
This revision now requires changes to proceed.

In D91053#2465584 <https://reviews.llvm.org/D91053#2465584>, @steven.zhang wrote:

> Rebase the patch.

Huh? Something wrong with this rebase? Not only is there an issue with `getSize()` I pointed out, but there are also uses of non-existent functions:

  PPCISelLowering.h:740:61: error: only virtual member functions can be marked 'override'
                              unsigned TargetFlags = 0) const override;
                                                              ^~~~~~~~
  PPCISelLowering.cpp:2997:12: error: no member named 'getConstantPool' in 'llvm::TargetLowering'; did you mean simply 'getConstantPool'?
      return TargetLowering::getConstantPool(C, DAG, NewAlign, VT, Alignment,
             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
             getConstantPool
  PPCISelLowering.cpp:2987:28: note: 'getConstantPool' declared here
  SDValue PPCTargetLowering::getConstantPool(const Constant *C, SelectionDAG &DAG,
                             ^
  PPCISelLowering.cpp:3018:12: error: no member named 'getConstantPool' in 'llvm::TargetLowering'; did you mean simply 'getConstantPool'?
      return TargetLowering::getConstantPool(C, DAG, NewAlign, VT, Alignment,
             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
             getConstantPool
  PPCISelLowering.cpp:2987:28: note: 'getConstantPool' declared here
  SDValue PPCTargetLowering::getConstantPool(const Constant *C, SelectionDAG &DAG,
                             ^
  3 errors generated.            

Also, I don't see adequate testing here (it may be hidden among all the test case changes and I missed it). There should be a test case with multiple constants loaded from the constant pool for all the types as well as for mixed types. AFAICT most of the test changes are single constants getting their own symbol in the TOC - which neither adequately tests this feature nor provides improvements of any kind (as pointed out in the description).



================
Comment at: llvm/lib/Target/PowerPC/PPCConstantPoolValue.h:28
+
+  const SmallVector<const Constant *, 8> getConstants() const {
+    return ConstantValues;
----------------
If the returned vector is `const`, why do we copy it?


================
Comment at: llvm/lib/Target/PowerPC/PPCConstantPoolValue.h:32
+
+  unsigned getSize(const DataLayout &DL) const override;
+
----------------
Is this an issue with the particular revision you developed this on? The base class doesn't have this as a virtual member function so compilation fails because of `override`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91053