Page MenuHomePhabricator

[NFC] Add the EmitTargetCodeForConstantPool hook for target to customize it with MachineConstantPoolValue
Needs ReviewPublic

Authored by steven.zhang on Nov 9 2020, 12:14 AM.

Details

Reviewers
MaskRay
RKSimon
echristo
spatel
efriedma
craig.topper
Group Reviewers
Restricted Project
Summary

We have the MachineConstantPoolValue to allow the target to customize the lowering of the constant. But when we are trying to create the Constant Pool from SelectionDAG, target has no way to customize it with MachineConstantPoolValue. So, propose a new hook inside SelectionDAGTargetInfo so that, target could customize the lower of constant pool. Also, add a new interface so that, we won't assume that, the result value of DAG.getConstantPool() must be ConstantPoolSDNode which is not always true if target customizes the lower of constant pool.

Diff Detail

Event Timeline

steven.zhang created this revision.Nov 9 2020, 12:14 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 9 2020, 12:14 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
steven.zhang requested review of this revision.Nov 9 2020, 12:14 AM

What should we do with SelectionDAG::getConstantPool? At the very least I'd expect better comments explaining when each should be used, but I'm wondering whether we can get away with just the TLI version?

yubing added a subscriber: yubing.Wed, Dec 23, 6:29 AM

What should we do with SelectionDAG::getConstantPool? At the very least I'd expect better comments explaining when each should be used, but I'm wondering whether we can get away with just the TLI version?

Removing the getConstantPool from SelectionDAG doesn't make sense as we are creating SDNode inside DAG. And yes, it will confuse us if there are two version, no matter how detail the comments have. Maybe, we can use the SelectionDAGTargetInfo and call the hook inside DAG.getConstantPool as what we did for getMcmcpy(). So that, we only have one interface. The challenge is that, there are many places that assume the return value of DAG.getConstantPool is ConstantPoolSDNode which will not be true anymore if there is hook inside it. I will try with this direction to see if there is a better way to handle this.

steven.zhang retitled this revision from [NFC] Add the getConstantPool hook for target to customize it with MachineConstantPoolValue to [NFC] Add the EmitTargetCodeForConstantPool hook for target to customize it with MachineConstantPoolValue.
steven.zhang edited the summary of this revision. (Show Details)

Re-implement with SelectionDAGTargetInfo.

Hmm... I think it is very surprising if DAG.getConstantPool() does not return ConstantPoolSDNode. I am not in favour of any design that violates such seemingly reasonable assumptions.

steven.zhang added a comment.EditedMon, Dec 28, 6:30 PM

Hmm... I think it is very surprising if DAG.getConstantPool() does not return ConstantPoolSDNode. I am not in favour of any design that violates such seemingly reasonable assumptions.

But it has no harm or even better to remove such assumptions. Is it right ?

The motivation for this patch is for D91053 (I should have documented it more clear). We indeed have the case that DAG.getConstantPool() doesn't return 'ConstantPoolSDNode'. This is what we want to do in PowerPC:

                  ConstantPoolSDNode
                  +----------------+
ConstantFP        |                |
+-------+         |                |
|  1.5  +--------->xxxxxxxxxxxxxxxx|
+-------+         |                |
                  |                |
                  |                |
ConstantFP        |                |
+-------+         |                |
|  3.0  +--------->xxxxxxxxxxxxxxxx|
+-------+         |                |
                  +----------------+

When we are creating the constant pool from DAG, we want to return the same ConstantPoolSDNode but with different offset. This is done by ConstantPoolSDNode + ADD. And as we have the MachineConstantPoolValue to allow target to customize its constant pool, we'd better remove such kind of assumption. And it also make the code a bit clean and extensible, IMO. What do you think ?

Ping...

I really don't think this is the right interface for this, similar to what Nemanja said. In addition, the uses and needs of Alignment here are a little odd and I think need some more explanation. Can you elaborate on what you're trying to do here a bit more?

Thanks!

-eric

steven.zhang added a comment.EditedTue, Jan 5, 9:49 PM
  • Reason why we need to change the interface

    I want to put the constants with the same type and alignment into the same constant pool(ConstantPoolSDNode) to reduce the TOC and improve the access performance. See the description in D91053 if you're interested in the detail. So, when we are trying to create the constant pool in DAG(DAG.getConstantPool), what it returns is the ADD ConstantPoolSDNode, Offset. And when emitting the ConstantPoolSDNode, we will have something like this(One TOC entry pointed to 4 constants in this example):
.LCPI0_0:
        .quad   0x402cc28f5c28f5c3              # double 14.380000000000001
        .quad   0x4002b851eb851eb8              # double 2.3399999999999999
        .quad   0x40120c49ba5e353f              # double 4.5119999999999996
        .quad   0x3ff3ae147ae147ae              # double 1.23

.LC0:
        .tc .LCPI0_0[TC],.LCPI0_0

As the return of DAG.getConstantPool() is NOT ConstantPoolSDNode anymore(It is shared ConstantPoolSDNode + Offset), we cannot get the alignment by casting the return value of DAG.getConstantPool() to ConstantPoolSDNode.

  • Reason why we change the interface that way

    The semantic of the interface of getConstantPool is that, if we specify the alignment, use it, otherwise, it will calculate the alignment for you. You have to get the alignment from the result of DAG.getConstantPool() if it is calculated internal which shows as follows:
SDValue SelectionDAG::getConstantPool(const Constant *C, EVT VT,
                                      MaybeAlign Alignment, int Offset,
                                      bool isTarget, unsigned TargetFlags) {
  assert((TargetFlags == 0 || isTarget) &&
         "Cannot set target flags on target-independent globals");
  if (!Alignment)
    Alignment = shouldOptForSize()
                    ? getDataLayout().getABITypeAlign(C->getType())
                    : getDataLayout().getPrefTypeAlign(C->getType());

So, I added a new parameter NewAlign to tell us what the final alignment constant pool has instead of casting from the return value that assume that, the return value must be ConstantPoolSDNode:

SDValue getConstantPool(const Constant *C, EVT VT, MaybeAlign Alignment, Align &NewAlign, ...)

DAG.getConstantPool(C, VT, None, NewAlign,...) means you don't have preference on the alignment and we will calculate it for you.
DAG.getConstantPool(C, VT, Align, NewAlign,...) means you have the alignment preference, but the final alignment is still set in NewAlign and they can be difference technical speaking. (i.e. you want the 4 byte aligned constant pool, we can still return the 8 byte aligned constant pool to share with it on PowerPC)

This is the way I am proposing and I am open and welcome for any suggestion. Thank you for all the comments and happy new year :)