Correctly set pointer bit for aggregate values in SelectionDAGBuilder to fix CCIfPtr #70554

camilstaps · 2023-10-28T13:26:10Z

See #70337 for discussion. There is a problem with the CCIfPtr check that can be used in calling conventions: it is not true for pointers in structs, as in:

define { ptr, i64 } @f(ptr %0, i64 %1) {
  %3 = insertvalue { ptr, i64 } undef, ptr %0, 0
  %4 = insertvalue { ptr, i64 } %3, i64 %1, 1
  ret { ptr, i64 } %4
}

In this PR ComputeValueVTs is extended to optionally return value types. This is then used in SelectionDAGBuilder to set the pointer bit in ArgFlags appropriately.

In #70337 (comment) it was suggested to extend ComputeValueVTs to return boolean IsPtrs. I have opted to let it return *Types instead, which I believe is equally efficient, but is a little bit more general in case these types are needed elsewhere in the future. I'd be happy to change this of course.

I tested this with the new calling convention I'm working on (see #70337). I am not sure about writing a test. On X86, CCIfPtr is only used in the C calling convention, to make sure that "Pointers are always passed in full 64-bit registers":

llvm-project/llvm/lib/Target/X86/X86CallingConv.td

Lines 570 to 571 in 497b2eb

    
           // Pointers are always passed in full 64-bit registers. 
        
           CCIfPtr<CCCustom<"CC_X86_64_Pointer">>,

llvm-project/llvm/lib/Target/X86/X86CallingConv.cpp

Lines 333 to 341 in 497b2eb

    
           static bool CC_X86_64_Pointer(unsigned &ValNo, MVT &ValVT, MVT &LocVT, 
        
                                         CCValAssign::LocInfo &LocInfo, 
        
                                         ISD::ArgFlagsTy &ArgFlags, CCState &State) { 
        
             if (LocVT != MVT::i64) { 
        
               LocVT = MVT::i64; 
        
               LocInfo = CCValAssign::ZExt; 
        
             } 
        
             return false; 
        
           }

It may be possible to use this to write a test, but I don't know what kind of IR would cause this rule to do anything meaningful: when would pointers not be passed in full 64-bit registers?

I am not that familiar with LLVM, so I apologize in advance for any and all problems with this PR.

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

camilstaps · 2023-10-28T13:42:06Z

There are failing tests (lld/test/ELF/lto/exclude-libs-libcall.ll and lld/test/ELF/lto/version-libcall.ll), but they do not contain pointer values and I don't see how they might be related. Could they be unrelated?

camilstaps · 2023-10-28T13:55:31Z

Also, in TargetLoweringBase I found one case where an ArgFlags struct is built for output arguments. Here setPointer is never used:

llvm-project/llvm/lib/CodeGen/TargetLoweringBase.cpp

Lines 1711 to 1758 in 497b2eb

    
           void llvm::GetReturnInfo(CallingConv::ID CC, Type *ReturnType, 
        
                                    AttributeList attr, 
        
                                    SmallVectorImpl<ISD::OutputArg> &Outs, 
        
                                    const TargetLowering &TLI, const DataLayout &DL) { 
        
             SmallVector<EVT, 4> ValueVTs; 
        
             ComputeValueVTs(TLI, DL, ReturnType, ValueVTs); 
        
             unsigned NumValues = ValueVTs.size(); 
        
             if (NumValues == 0) return; 
        
             for (unsigned j = 0, f = NumValues; j != f; ++j) { 
        
               EVT VT = ValueVTs[j]; 
        
               ISD::NodeType ExtendKind = ISD::ANY_EXTEND; 
        
               if (attr.hasRetAttr(Attribute::SExt)) 
        
                 ExtendKind = ISD::SIGN_EXTEND; 
        
               else if (attr.hasRetAttr(Attribute::ZExt)) 
        
                 ExtendKind = ISD::ZERO_EXTEND; 
        
               // FIXME: C calling convention requires the return type to be promoted to 
        
               // at least 32-bit. But this is not necessary for non-C calling 
        
               // conventions. The frontend should mark functions whose return values 
        
               // require promoting with signext or zeroext attributes. 
        
               if (ExtendKind != ISD::ANY_EXTEND && VT.isInteger()) { 
        
                 MVT MinVT = TLI.getRegisterType(MVT::i32); 
        
                 if (VT.bitsLT(MinVT)) 
        
                   VT = MinVT; 
        
               } 
        
               unsigned NumParts = 
        
                   TLI.getNumRegistersForCallingConv(ReturnType->getContext(), CC, VT); 
        
               MVT PartVT = 
        
                   TLI.getRegisterTypeForCallingConv(ReturnType->getContext(), CC, VT); 
        
               // 'inreg' on function refers to return value 
        
               ISD::ArgFlagsTy Flags = ISD::ArgFlagsTy(); 
        
               if (attr.hasRetAttr(Attribute::InReg)) 
        
                 Flags.setInReg(); 
        
               // Propagate extension type if any 
        
               if (attr.hasRetAttr(Attribute::SExt)) 
        
                 Flags.setSExt(); 
        
               else if (attr.hasRetAttr(Attribute::ZExt)) 
        
                 Flags.setZExt(); 
        
               for (unsigned i = 0; i < NumParts; ++i) 
        
                 Outs.push_back(ISD::OutputArg(Flags, PartVT, VT, /*isfixed=*/true, 0, 0)); 
        
             } 
        
           }

I am not sure whether this is intentional, or an omission. I could add it if desired, but I was not sure when this function is used, exactly.

arsenm · 2023-10-30T08:16:14Z

Also, in TargetLoweringBase I found one case where an ArgFlags struct is built for output arguments. Here setPointer is never used:

llvm-project/llvm/lib/CodeGen/TargetLoweringBase.cpp

Lines 1711 to 1758 in 497b2eb

void llvm::GetReturnInfo(CallingConv::ID CC, Type *ReturnType,

AttributeList attr,

SmallVectorImpl<ISD::OutputArg> &Outs,

const TargetLowering &TLI, const DataLayout &DL) {

SmallVector<EVT, 4> ValueVTs;

ComputeValueVTs(TLI, DL, ReturnType, ValueVTs);

unsigned NumValues = ValueVTs.size();

if (NumValues == 0) return;

for (unsigned j = 0, f = NumValues; j != f; ++j) {

EVT VT = ValueVTs[j];

ISD::NodeType ExtendKind = ISD::ANY_EXTEND;

if (attr.hasRetAttr(Attribute::SExt))

ExtendKind = ISD::SIGN_EXTEND;

else if (attr.hasRetAttr(Attribute::ZExt))

ExtendKind = ISD::ZERO_EXTEND;

// FIXME: C calling convention requires the return type to be promoted to

// at least 32-bit. But this is not necessary for non-C calling

// conventions. The frontend should mark functions whose return values

// require promoting with signext or zeroext attributes.

if (ExtendKind != ISD::ANY_EXTEND && VT.isInteger()) {

MVT MinVT = TLI.getRegisterType(MVT::i32);

if (VT.bitsLT(MinVT))

VT = MinVT;

}

unsigned NumParts =

TLI.getNumRegistersForCallingConv(ReturnType->getContext(), CC, VT);

MVT PartVT =

TLI.getRegisterTypeForCallingConv(ReturnType->getContext(), CC, VT);

// 'inreg' on function refers to return value

ISD::ArgFlagsTy Flags = ISD::ArgFlagsTy();

if (attr.hasRetAttr(Attribute::InReg))

Flags.setInReg();

// Propagate extension type if any

if (attr.hasRetAttr(Attribute::SExt))

Flags.setSExt();

else if (attr.hasRetAttr(Attribute::ZExt))

Flags.setZExt();

for (unsigned i = 0; i < NumParts; ++i)

Outs.push_back(ISD::OutputArg(Flags, PartVT, VT, /*isfixed=*/true, 0, 0));

}

}

I am not sure whether this is intentional, or an omission. I could add it if desired, but I was not sure when this function is used, exactly.

The isPointer field is a relatively new hack, most places probably were never updated to set it without a motivating example. It should be consistently respected. Ideally we would move all of this code to use LLT to ease the GlobalISel transition which would directly preserve the pointeriness.

This version looks like it's mostly for FastISel. PPC seems to be using it for tail call eligibility checks, and AArch64 seems to use it to set one field in MachineFunctionInfo (which I think it could avoid)

arsenm · 2023-10-30T08:25:28Z

llvm/include/llvm/CodeGen/Analysis.h

@@ -75,20 +75,23 @@ void ComputeValueVTs(const TargetLowering &TLI, const DataLayout &DL, Type *Ty,
                     SmallVectorImpl<uint64_t> *FixedOffsets,
                     uint64_t StartingOffset);

-/// Variant of ComputeValueVTs that also produces the memory VTs.
+/// Variant of ComputeValueVTs that also produces the memory VTs and VT types.


I don't particularly like using a new output for the Types. The Types should be redundant with EVT (which sometimes do contain a Type* reference). The current hack unnaturally separates the pointer information in the ArgFlags. I believe you can always infer the pointer type from the underlying IR argument at the use point, although that will be really annoying for nested structs.

Long term we should have one of these that returns LLTs instead of EVT, that will preserve the pointer information.

@arsenm Thanks for the review! If I'm understanding the code correctly, EVTs do contain Type* references, but the EVT created for pointers in ComputeValueVTs is an integer type (MVT::getIntegerVT(DL.getPointerSizeInBits(AS))). I don't see a way to easily use the LLVMTy in EVTs for this purpose, but I may very well be mistaken.

It is possible to remove this logic from ComputeValueVTs and have a separate function to flatten the IR type. But there is quite a number of cases where this needs to be done, and the logic is identical to the one in ComputeValueVTs. If you think it preferable to do this, I would be happy to implement it, but I just want to make sure I'm understanding you correctly.

I would also be happy to implement a ComputeValueVTs variant for LLTs if you're going to need that for other things anyway. I don't really know what the GlobalISel transition is though and how involved this would be.

I'm assuming returning booleans (true for pointers) rather than Type*s from ComputeValueVTs does not really make things better for you? This was suggested by phoebewang in the linked issue.

@arsenm If you have time, could you still have a look and let me know what approach you'd prefer?

Right, the EVTs do unfortunately squash away the pointer types (because they also resolve to integer MVTs, the Type* is dropped). I meant more the other direction, would it work to change this to return the aggregate split Type s instead of the EVT? The client would then have a thin Type->EVT wrapper

github-actions · 2023-11-03T20:48:51Z

✅ With the latest revision this PR passed the C/C++ code formatter.

… to fix CCIfPtr

arsenm · 2023-12-01T03:12:44Z

llvm/lib/CodeGen/SelectionDAG/FastISel.cpp

+      if (Ty->isPointerTy()) {
+        MyFlags.Flags.setPointer();
+        MyFlags.Flags.setPointerAddrSpace(
+            cast<PointerType>(Ty)->getAddressSpace());


dyn_cast to PointerType instead of isPointerTy+cast (though I guess this is already pre-existing in the other cases)

arsenm · 2023-12-01T03:17:32Z

llvm/include/llvm/CodeGen/Analysis.h

@@ -75,20 +75,23 @@ void ComputeValueVTs(const TargetLowering &TLI, const DataLayout &DL, Type *Ty,
                     SmallVectorImpl<uint64_t> *FixedOffsets,
                     uint64_t StartingOffset);

-/// Variant of ComputeValueVTs that also produces the memory VTs.
+/// Variant of ComputeValueVTs that also produces the memory VTs and VT types.


Right, the EVTs do unfortunately squash away the pointer types (because they also resolve to integer MVTs, the Type* is dropped). I meant more the other direction, would it work to change this to return the aggregate split Type s instead of the EVT? The client would then have a thin Type->EVT wrapper

camilstaps commented Oct 28, 2023

View reviewed changes

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp Show resolved Hide resolved

camilstaps marked this pull request as ready for review October 28, 2023 13:42

llvmbot added the llvm:SelectionDAG SelectionDAGISel as well label Oct 28, 2023

camilstaps mentioned this pull request Oct 28, 2023

CCIfPtr appears to always be false in return calling conventions #70337

Open

arsenm reviewed Oct 30, 2023

View reviewed changes

camilstaps force-pushed the fix-CCIfPtr-for-aggregate-return-values branch from a200429 to efeef7b Compare November 3, 2023 20:39

Correctly set pointer bit for aggregate values in SelectionDAGBuilder…

36d218b

… to fix CCIfPtr

camilstaps force-pushed the fix-CCIfPtr-for-aggregate-return-values branch from efeef7b to 36d218b Compare November 3, 2023 20:52

arsenm reviewed Dec 1, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Correctly set pointer bit for aggregate values in SelectionDAGBuilder to fix CCIfPtr #70554

Correctly set pointer bit for aggregate values in SelectionDAGBuilder to fix CCIfPtr #70554

camilstaps commented Oct 28, 2023 •

edited

camilstaps commented Oct 28, 2023

camilstaps commented Oct 28, 2023

arsenm commented Oct 30, 2023

arsenm Oct 30, 2023

camilstaps Nov 3, 2023

camilstaps Nov 30, 2023

arsenm Dec 1, 2023

github-actions bot commented Nov 3, 2023 •

edited

arsenm Dec 1, 2023

arsenm Dec 1, 2023

	// Pointers are always passed in full 64-bit registers.
	CCIfPtr<CCCustom<"CC_X86_64_Pointer">>,

	static bool CC_X86_64_Pointer(unsigned &ValNo, MVT &ValVT, MVT &LocVT,
	CCValAssign::LocInfo &LocInfo,
	ISD::ArgFlagsTy &ArgFlags, CCState &State) {
	if (LocVT != MVT::i64) {
	LocVT = MVT::i64;
	LocInfo = CCValAssign::ZExt;
	}
	return false;
	}

Correctly set pointer bit for aggregate values in SelectionDAGBuilder to fix CCIfPtr #70554

Are you sure you want to change the base?

Correctly set pointer bit for aggregate values in SelectionDAGBuilder to fix CCIfPtr #70554

Conversation

camilstaps commented Oct 28, 2023 • edited

camilstaps commented Oct 28, 2023

camilstaps commented Oct 28, 2023

arsenm commented Oct 30, 2023

arsenm Oct 30, 2023

Choose a reason for hiding this comment

camilstaps Nov 3, 2023

Choose a reason for hiding this comment

camilstaps Nov 30, 2023

Choose a reason for hiding this comment

arsenm Dec 1, 2023

Choose a reason for hiding this comment

github-actions bot commented Nov 3, 2023 • edited

arsenm Dec 1, 2023

Choose a reason for hiding this comment

arsenm Dec 1, 2023

Choose a reason for hiding this comment

camilstaps commented Oct 28, 2023 •

edited

github-actions bot commented Nov 3, 2023 •

edited