This is an archive of the discontinued LLVM Phabricator instance.

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

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

Details

Reviewers
MaskRay
RKSimon
echristo
spatel
efriedma
craig.topper
steven.zhang
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.Dec 23 2020, 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.EditedDec 28 2020, 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.EditedJan 5 2021, 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 :)

Passing an alignment in and returning an alignment out at the same time seems unlikely to be useful. DAGCombine can refine the alignment later anyway, if it turns out to be relevant. Maybe rename the version that returns the alignment out?

Also, if the point is literally just "glue a bunch of ConstantPools together", can we teach target-independent code to do that? It doesn't actually require anything target-specific, I think, and it should be useful on any target where computing the address of a constant pool isn't cheap.

(Just realized this patch isn't being actively developed; feel free to ignore if you're not planning to continue work on this.)

lkail commandeered this revision.Jul 28 2021, 7:58 PM
lkail added a reviewer: steven.zhang.
lkail abandoned this revision.Jul 28 2021, 8:04 PM

(Just realized this patch isn't being actively developed; feel free to ignore if you're not planning to continue work on this.)

This patch is in preparation for https://reviews.llvm.org/D91053 which aims at optimizing layout of PPC's TOC. Current implementation still has limitations. To me, at first galance, we should just collect what constants we have at each basic block at isel phase and perform TOC layout optimization after isel.

lkail added a comment.EditedJul 28 2021, 8:13 PM

Sorry, I just missed this.

Also, if the point is literally just "glue a bunch of ConstantPools together", can we teach target-independent code to do that? It doesn't actually require anything target-specific, I think, and it should be useful on any target where computing the address of a constant pool isn't cheap.

"glue a bunch of ConstantPools together", exactly.

can we teach target-independent code to do that? It doesn't actually require anything target-specific, I think, and it should be useful on any target where computing the address of a constant pool isn't cheap.

Good one, maybe we would post a new patch for this.

Passing an alignment in and returning an alignment out at the same time seems unlikely to be useful. DAGCombine can refine the alignment later anyway, if it turns out to be relevant. Maybe rename the version that returns the alignment out?

Also, if the point is literally just "glue a bunch of ConstantPools together", can we teach target-independent code to do that? It doesn't actually require anything target-specific, I think, and it should be useful on any target where computing the address of a constant pool isn't cheap.

Passing a constant in,which has the alignment of the constant,return a constantpool,which need to specify the alignment of constantpool. We cannot query the alignment of constantpool now as the result could be cp + offset.

Passing an alignment in and returning an alignment out at the same time seems unlikely to be useful. DAGCombine can refine the alignment later anyway, if it turns out to be relevant. Maybe rename the version that returns the alignment out?

Passing a constant in,which has the alignment of the constant,return a constantpool,which need to specify the alignment of constantpool. We cannot query the alignment of constantpool now as the result could be cp + offset.

I think you're missing my point. If the caller has a particular idea of what alignment it needs, it will request it. The result will have at least that alignment. If the actual alignment comes out higher, it doesn't really matter.

SelectionDAG::computeKnownBits can be used to query the alignment of an arbitrary pointer. It looks like it doesn't currently try to use the alignment of a constant pool, but it could, if it mattered.

Passing an alignment in and returning an alignment out at the same time seems unlikely to be useful. DAGCombine can refine the alignment later anyway, if it turns out to be relevant. Maybe rename the version that returns the alignment out?

Passing a constant in,which has the alignment of the constant,return a constantpool,which need to specify the alignment of constantpool. We cannot query the alignment of constantpool now as the result could be cp + offset.

I think you're missing my point. If the caller has a particular idea of what alignment it needs, it will request it. The result will have at least that alignment. If the actual alignment comes out higher, it doesn't really matter.

SelectionDAG::computeKnownBits can be used to query the alignment of an arbitrary pointer. It looks like it doesn't currently try to use the alignment of a constant pool, but it could, if it mattered.

Great to know that we have some way to query the alignment. If that works, we could have one patch to avoid querying the constant pool alignment again after it is created. And get the underlying alignment with computeKnowBits likewise interface in the place that need the exactly alignment.