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

[Qing Shan Zhang via Phabricator via llvm-commits]

steven.zhang added a comment.

In D91053#2466091 <https://reviews.llvm.org/D91053#2466091>, @nemanjai wrote:

> 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).

There are two parent revisions that this patch depends on. You need to apply them first. I will add a new test to summarize all the pattern we can handle.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91053