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

[lkail]

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.

[steven.zhang]

Gentle ping.

[RKSimon]

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?

[steven.zhang]

In D91050#2469885, @RKSimon wrote:

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]

Re-implement with SelectionDAGTargetInfo.

[nemanjai]

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]

In D91050#2472648, @nemanjai wrote:

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 ?

[steven.zhang]

Ping...

[echristo]

In D91050#2480906, @steven.zhang wrote:

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]

• 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 :)

[efriedma]

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.

[efriedma]

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

[lkail]

In D91050#2905388, @efriedma wrote:

(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]

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.

[steven.zhang]

In D91050#2905380, @efriedma wrote:

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.

[efriedma]

In D91050#2915306, @steven.zhang wrote:

In D91050#2905380, @efriedma wrote:

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.

[steven.zhang]

In D91050#2917401, @efriedma wrote:

In D91050#2915306, @steven.zhang wrote:

In D91050#2905380, @efriedma wrote:

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.

llvm/include/llvm/CodeGen/TargetLowering.h

public:
   /// physical registers in the exit blocks. This function is called at the end
   /// of instruction selection.
   virtual void insertCopiesSplitCSR(
       MachineBasicBlock *Entry,
       const SmallVectorImpl<MachineBasicBlock *> &Exits) const {
     llvm_unreachable("Not Implemented");
   }
 
+  /// Create target specific constant pool and set the new alignment in
+  /// \p NewAlign
+  virtual SDValue getConstantPool(const Constant *C, SelectionDAG &DAG,
+                                  Align &NewAlign, EVT VT,
+                                  MaybeAlign Align = None, int Offset = 0,
+                                  bool isTarget = false,
+                                  unsigned TargetFlags = 0) const {
+    SDValue CPIdx =
+        DAG.getConstantPool(C, VT, Align, Offset, isTarget, TargetFlags);
+    NewAlign = cast<ConstantPoolSDNode>(CPIdx)->getAlign();
+    return CPIdx;
+  }
+
   /// Return the newly negated expression if the cost is not expensive and
   /// set the cost in \p Cost to indicate that if it is cheaper or neutral to
   /// do the negation.
   virtual SDValue getNegatedExpression(SDValue Op, SelectionDAG &DAG,
                                        bool LegalOps, bool OptForSize,
                                        NegatibleCost &Cost,
                                        unsigned Depth = 0) const;
 

llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp

while (SVT != MVT::f32 && SVT != MVT::f16) {
         Type *SType = SVT.getTypeForEVT(*DAG.getContext());
         LLVMC = cast<ConstantFP>(ConstantExpr::getFPTrunc(LLVMC, SType));
         VT = SVT;
         Extend = true;
       }
     }
   }
 
-  SDValue CPIdx =
-      DAG.getConstantPool(LLVMC, TLI.getPointerTy(DAG.getDataLayout()));
-  Align Alignment = cast<ConstantPoolSDNode>(CPIdx)->getAlign();
+  Align Alignment;
+  SDValue CPIdx = TLI.getConstantPool(LLVMC, DAG, Alignment,
+                                      TLI.getPointerTy(DAG.getDataLayout()));
   if (Extend) {
     SDValue Result = DAG.getExtLoad(
         ISD::EXTLOAD, dl, OrigVT, DAG.getEntryNode(), CPIdx,
         MachinePointerInfo::getConstantPool(DAG.getMachineFunction()), VT,
         Alignment);
     return Result;
   }
   SDValue Result = DAG.getLoad(
       OrigVT, dl, DAG.getEntryNode(), CPIdx,
       MachinePointerInfo::getConstantPool(DAG.getMachineFunction()), Alignment);
   return Result;
 }
 
 /// Expands the Constant node to a load from the constant pool.
 SDValue SelectionDAGLegalize::ExpandConstant(ConstantSDNode *CP) {
   SDLoc dl(CP);
+  Align Alignment;
   EVT VT = CP->getValueType(0);
-  SDValue CPIdx = DAG.getConstantPool(CP->getConstantIntValue(),
+  SDValue CPIdx = TLI.getConstantPool(CP->getConstantIntValue(), DAG, Alignment,
                                       TLI.getPointerTy(DAG.getDataLayout()));
-  Align Alignment = cast<ConstantPoolSDNode>(CPIdx)->getAlign();
   SDValue Result = DAG.getLoad(
       VT, dl, DAG.getEntryNode(), CPIdx,
       MachinePointerInfo::getConstantPool(DAG.getMachineFunction()), Alignment);
   return Result;
 }
 
 /// Some target cannot handle a variable insertion index for the
 /// INSERT_VECTOR_ELT instruction.  In this case, it
for (unsigned i = 0, e = NumElems; i != e; ++i) {
                                         CI->getZExtValue()));
         }
       } else {
         assert(Node->getOperand(i).isUndef());
         Type *OpNTy = EltVT.getTypeForEVT(*DAG.getContext());
         CV.push_back(UndefValue::get(OpNTy));
       }
     }
+    Align Alignment;
     Constant *CP = ConstantVector::get(CV);
-    SDValue CPIdx =
-        DAG.getConstantPool(CP, TLI.getPointerTy(DAG.getDataLayout()));
-    Align Alignment = cast<ConstantPoolSDNode>(CPIdx)->getAlign();
+    SDValue CPIdx = TLI.getConstantPool(CP, DAG, Alignment,
+                                        TLI.getPointerTy(DAG.getDataLayout()));
     return DAG.getLoad(
         VT, dl, DAG.getEntryNode(), CPIdx,
         MachinePointerInfo::getConstantPool(DAG.getMachineFunction()),
         Alignment);
   }
 
   SmallSet<SDValue, 16> DefinedValues;
   for (unsigned i = 0; i < NumElems; ++i) {
SDValue SelectionDAGLegalize::ExpandLegalINT_TO_FP(SDNode *Node,
   case MVT::i32: FF = 0x4F800000ULL; break;  // 2^32 (as a float)
   case MVT::i64: FF = 0x5F800000ULL; break;  // 2^64 (as a float)
   }
   if (DAG.getDataLayout().isLittleEndian())
     FF <<= 32;
   Constant *FudgeFactor = ConstantInt::get(
                                        Type::getInt64Ty(*DAG.getContext()), FF);
 
-  SDValue CPIdx =
-      DAG.getConstantPool(FudgeFactor, TLI.getPointerTy(DAG.getDataLayout()));
-  Align Alignment = cast<ConstantPoolSDNode>(CPIdx)->getAlign();
+  Align Alignment;
+  SDValue CPIdx = TLI.getConstantPool(FudgeFactor, DAG, Alignment,
+                                      TLI.getPointerTy(DAG.getDataLayout()));
   CPIdx = DAG.getNode(ISD::ADD, dl, CPIdx.getValueType(), CPIdx, CstOffset);
   Alignment = commonAlignment(Alignment, 4);
   SDValue FudgeInReg;
   if (DestVT == MVT::f32)
     FudgeInReg = DAG.getLoad(
         MVT::f32, dl, DAG.getEntryNode(), CPIdx,
         MachinePointerInfo::getConstantPool(DAG.getMachineFunction()),
         Alignment);