TargetMachine: Add address space to getPointerSize

[arsenm]

Summary

I'm not sure why these wrappers are here rather than just exposing the contained DataLayout

[bjope]

include/llvm/Target/TargetMachine.h

141–155

Can we skip the default =0 for these?

If I remember correclty there are FIXME comments in DataLayout about removing the defaults in DataLayout. I think the idea is that people often forget to provide the correct address space. If an address space is mandatory one needs to motivate at a specific call site if it for example is correct to always use 0 instead of looking up the address space for the type. The amount of call sites where it always is correct to use a hard-coded 0 is probably very few.

[arsenm]

include/llvm/Target/TargetMachine.h

141–155

I would like to remove the default, but the context where this was used I don't see where the pointer is coming from (i.e. a few places AsmPrinter).

I'm doing this because it seems like the TargetMachine is the natural place to check the pointer size in GlobalISel's LegalizeInfo

[bjope]

include/llvm/Target/TargetMachine.h

141–155

My idea was to, for example, replace all old calls to this method into:
getPointerSize(0 /* TODO: Check/describe why address space 0 is correct here*/)
and, by that removing the default setting, and pushing the TODO down to the call sites.

[arsenm]

Remove default. I did my best to guess at the context these were used for. Some of the DebugInfo ones are questionable, as well as AsmPrinter::getPointerSize

[bjope]

Thanks Matt! I like this version without the default argument.

I think your guesses about correct address spaces are as good as mine. But I'm not 100% sure about a few diffs where we now use the "program AS" instead of AS=0. See my inline comments.

lib/CodeGen/AsmPrinter/AsmPrinter.cpp

218–220

As far as I can see this determines size of dwarf::DW_FORM_ref_addr for DWARF2, as it is used from DIEInteger::SizeOf().
(It is also used in test code in unittests/DebugInfo/DWARF/DwarfGenerator.cpp)

In DWARF2 DW_FORM_ref_addr was (confusingly) defined as being the size of an address on the target machine. Not sure what that means if there are several address spaces with different sizes on the target machine.

In our out-of-tree-target (a DSP) we have getPointerSize(0) != getProgramPointerSize(). Although we do not use DWARF2, so I do not think it really matter for us...

You could play it safe and simply do TM.getPointerSize(0), to avoid any functional change in this patch. But using TM.getProgramPointerSize() might actually be more correct. I do not really know.

984

Seems reasonable to me.

lib/CodeGen/AsmPrinter/CodeViewDebug.cpp

1529–1530

Maybe one needs to inspect Ty here to really find out which address space that should be used.
I'd leave it as Asm->TM.getPointerSize(0) if being uncertain (to avoid changing behavior by mistake).

[arsenm]

lib/CodeGen/AsmPrinter/AsmPrinter.cpp

218–220

I think this needs getDWARFAddressSpace, but that may need a mapping to the target address space

[arsenm]

Leave 0 in another place

[bjope]

LGTM

[arsenm]

r327467

include/llvm/Target/TargetMachine.h

public:
   /// check on the validity of this DataLayout.
   bool isCompatibleDataLayout(const DataLayout &Candidate) const {
     return DL == Candidate;
   }
 
   /// Get the pointer size for this target.
   ///
   /// This is the only time the DataLayout in the TargetMachine is used.
-  unsigned getPointerSize() const { return DL.getPointerSize(); }
+  unsigned getPointerSize(unsigned AS) const {
+    return DL.getPointerSize(AS);
+  }
+
+  unsigned getPointerSizeInBits(unsigned AS) const {
+    return DL.getPointerSizeInBits(AS);
+  }
+
+  unsigned getProgramPointerSize() const {
+    return DL.getPointerSize(DL.getProgramAddressSpace());
+  }
+
+  unsigned getAllocaPointerSize() const {
+    return DL.getPointerSize(DL.getAllocaAddrSpace());
+  }
 
   /// \brief Reset the target options based on the function's attributes.
   // FIXME: Remove TargetOptions that affect per-function code generation
   // from TargetMachine.
   void resetTargetOptions(const Function &F) const;
 
   /// Return target specific asm information.
   const MCAsmInfo *getMCAsmInfo() const { return AsmInfo; }

lib/CodeGen/AsmPrinter/AsmPrinter.cpp

 }
 
 const DataLayout &AsmPrinter::getDataLayout() const {
   return MMI->getModule()->getDataLayout();
 }
 
 // Do not use the cached DataLayout because some client use it without a Module
 // (llvm-dsymutil, llvm-dwarfdump).
-unsigned AsmPrinter::getPointerSize() const { return TM.getPointerSize(); }
+unsigned AsmPrinter::getPointerSize() const {
+  return TM.getPointerSize(0); // FIXME: Default address space
+}
 
 const MCSubtargetInfo &AsmPrinter::getSubtargetInfo() const {
   assert(MF && "getSubtargetInfo requires a valid MachineFunction!");
   return MF->getSubtarget<MCSubtargetInfo>();
 }
 
 void AsmPrinter::EmitToStreamer(MCStreamer &S, const MCInst &Inst) {
   S.EmitInstruction(Inst, getSubtargetInfo());
void AsmPrinter::emitStackSizeSection(const MachineFunction &MF) {
   if (FrameInfo.hasVarSizedObjects())
     return;
 
   OutStreamer->PushSection();
   OutStreamer->SwitchSection(StackSizeSection);
 
   const MCSymbol *FunctionSymbol = getSymbol(&MF.getFunction());
   uint64_t StackSize = FrameInfo.getStackSize();
-  OutStreamer->EmitSymbolValue(FunctionSymbol, TM.getPointerSize());
+  OutStreamer->EmitSymbolValue(FunctionSymbol, TM.getProgramPointerSize());
   OutStreamer->EmitULEB128IntValue(StackSize);
 
   OutStreamer->PopSection();
 }
 
 static bool needFuncLabelsForEHOrDebugInfo(const MachineFunction &MF,
                                            MachineModuleInfo *MMI) {
   if (!MF.getLandingPads().empty() || MF.hasEHFunclets() || MMI->hasDebugInfo())

lib/CodeGen/AsmPrinter/CodeViewDebug.cpp

TypeIndex CodeViewDebug::lowerTypeAlias(const DIDerivedType *Ty) {
 
   return UnderlyingTypeIndex;
 }
 
 TypeIndex CodeViewDebug::lowerTypeArray(const DICompositeType *Ty) {
   DITypeRef ElementTypeRef = Ty->getBaseType();
   TypeIndex ElementTypeIndex = getTypeIndex(ElementTypeRef);
   // IndexType is size_t, which depends on the bitness of the target.
-  TypeIndex IndexType = Asm->TM.getPointerSize() == 8
+  TypeIndex IndexType = getPointerSizeInBytes() == 8
                             ? TypeIndex(SimpleTypeKind::UInt64Quad)
                             : TypeIndex(SimpleTypeKind::UInt32Long);
 
   uint64_t ElementSize = getBaseTypeSize(ElementTypeRef) / 8;
 
   // Add subranges to array type.
   DINodeArray Elements = Ty->getElements();
   for (int i = Elements.size() - 1; i >= 0; --i) {
translatePtrToMemberRep(unsigned SizeInBytes, bool IsPMF, unsigned Flags) {
   llvm_unreachable("invalid ptr to member representation");
 }
 
 TypeIndex CodeViewDebug::lowerTypeMemberPointer(const DIDerivedType *Ty,
                                                 PointerOptions PO) {
   assert(Ty->getTag() == dwarf::DW_TAG_ptr_to_member_type);
   TypeIndex ClassTI = getTypeIndex(Ty->getClassType());
   TypeIndex PointeeTI = getTypeIndex(Ty->getBaseType(), Ty->getClassType());
-  PointerKind PK = Asm->TM.getPointerSize() == 8 ? PointerKind::Near64
-                                                 : PointerKind::Near32;
+  PointerKind PK = getPointerSizeInBytes() == 8 ? PointerKind::Near64
   bool IsPMF = isa<DISubroutineType>(Ty->getBaseType());
   PointerMode PM = IsPMF ? PointerMode::PointerToMemberFunction
                          : PointerMode::PointerToDataMember;
 
   assert(Ty->getSizeInBits() / 8 <= 0xff && "pointer size too big");
   uint8_t SizeInBytes = Ty->getSizeInBits() / 8;
   MemberPointerInfo MPI(
       ClassTI, translatePtrToMemberRep(SizeInBytes, IsPMF, Ty->getFlags()));

lib/CodeGen/TargetFrameLoweringImpl.cpp

void TargetFrameLowering::determineCalleeSaves(MachineFunction &MF,
   }
 }
 
 unsigned TargetFrameLowering::getStackAlignmentSkew(
     const MachineFunction &MF) const {
   // When HHVM function is called, the stack is skewed as the return address
   // is removed from the stack before we enter the function.
   if (LLVM_UNLIKELY(MF.getFunction().getCallingConv() == CallingConv::HHVM))
-    return MF.getTarget().getPointerSize();
+    return MF.getTarget().getAllocaPointerSize();
 
   return 0;
 }

lib/Target/ARM/ARMInstructionSelector.cpp

if (GV->isThreadLocal()) {
     return false;
   }
 
   auto &MBB = *MIB->getParent();
   auto &MF = *MBB.getParent();
 
   bool UseMovt = STI.useMovt(MF);
 
-  unsigned Size = TM.getPointerSize();
+  unsigned Size = TM.getPointerSize(0);
   unsigned Alignment = 4;
 
   auto addOpsForConstantPoolLoad = [&MF, Alignment,
                                     Size](MachineInstrBuilder &MIB,
                                           const GlobalValue *GV, bool IsSBREL) {
     assert(MIB->getOpcode() == ARM::LDRi12 && "Unsupported instruction");
     auto ConstPool = MF.getConstantPool();
     auto CPIndex =
if (STI.isTargetDarwin())
       TargetFlags |= ARMII::MO_NONLAZY;
     if (STI.isGVInGOT(GV))
       TargetFlags |= ARMII::MO_GOT;
     MIB->getOperand(1).setTargetFlags(TargetFlags);
 
     if (Indirect)
       MIB.addMemOperand(MF.getMachineMemOperand(
           MachinePointerInfo::getGOT(MF), MachineMemOperand::MOLoad,
-          TM.getPointerSize(), Alignment));
+          TM.getProgramPointerSize(), Alignment));
 
     return constrainSelectedInstRegOperands(*MIB, TII, TRI, RBI);
   }
 
   bool isReadOnly = STI.getTargetLowering()->isReadOnly(GV);
   if (STI.isROPI() && isReadOnly) {
     unsigned Opc = UseMovt ? ARM::MOV_ga_pcrel : ARM::LDRLIT_ga_pcrel;
     MIB->setDesc(TII.get(Opc));

lib/Target/Sparc/SparcISelLowering.cpp

static SPCC::CondCodes FPCondCCodeToFCC(ISD::CondCode CC) {
   case ISD::SETONE: return SPCC::FCC_LG;
   case ISD::SETUEQ: return SPCC::FCC_UE;
   }
 }
 
 SparcTargetLowering::SparcTargetLowering(const TargetMachine &TM,
                                          const SparcSubtarget &STI)
     : TargetLowering(TM), Subtarget(&STI) {
-  MVT PtrVT = MVT::getIntegerVT(8 * TM.getPointerSize());
+  MVT PtrVT = MVT::getIntegerVT(8 * TM.getPointerSize(0));
 
   // Instructions which use registers as conditionals examine all the
   // bits (as does the pseudo SELECT_CC expansion). I don't think it
   // matters much whether it's ZeroOrOneBooleanContent, or
   // ZeroOrNegativeOneBooleanContent, so, arbitrarily choose the
   // former.
   setBooleanContents(ZeroOrOneBooleanContent);
   setBooleanVectorContents(ZeroOrOneBooleanContent);

lib/Target/SystemZ/SystemZISelLowering.cpp

static MachineOperand earlyUseOperand(MachineOperand Op) {
   if (Op.isReg())
     Op.setIsKill(false);
   return Op;
 }
 
 SystemZTargetLowering::SystemZTargetLowering(const TargetMachine &TM,
                                              const SystemZSubtarget &STI)
     : TargetLowering(TM), Subtarget(STI) {
-  MVT PtrVT = MVT::getIntegerVT(8 * TM.getPointerSize());
+  MVT PtrVT = MVT::getIntegerVT(8 * TM.getPointerSize(0));
 
   // Set up the register classes.
   if (Subtarget.hasHighWord())
     addRegisterClass(MVT::i32, &SystemZ::GRX32BitRegClass);
   else
     addRegisterClass(MVT::i32, &SystemZ::GR32BitRegClass);
   addRegisterClass(MVT::i64, &SystemZ::GR64BitRegClass);
   if (Subtarget.hasVector()) {

lib/Target/X86/X86ISelLowering.cpp

 }
 
 X86TargetLowering::X86TargetLowering(const X86TargetMachine &TM,
                                      const X86Subtarget &STI)
     : TargetLowering(TM), Subtarget(STI) {
   bool UseX87 = !Subtarget.useSoftFloat() && Subtarget.hasX87();
   X86ScalarSSEf64 = Subtarget.hasSSE2();
   X86ScalarSSEf32 = Subtarget.hasSSE1();
-  MVT PtrVT = MVT::getIntegerVT(8 * TM.getPointerSize());
+  MVT PtrVT = MVT::getIntegerVT(TM.getPointerSizeInBits(0));
 
   // Set up the TargetLowering object.
 
   // X86 is weird. It always uses i8 for shift amounts and setcc results.
   setBooleanContents(ZeroOrOneBooleanContent);
   // X86-SSE is even stranger. It uses -1 or 0 for vector masks.
   setBooleanVectorContents(ZeroOrNegativeOneBooleanContent);
 

lib/Target/X86/X86LegalizerInfo.cpp

X86LegalizerInfo::X86LegalizerInfo(const X86Subtarget &STI,
   setLegalizeScalarToDifferentSizeStrategy(
       G_CONSTANT, 0, widenToLargerTypesAndNarrowToLargest);
 
   computeTables();
 }
 
 void X86LegalizerInfo::setLegalizerInfo32bit() {
 
-  const LLT p0 = LLT::pointer(0, TM.getPointerSize() * 8);
+  const LLT p0 = LLT::pointer(0, TM.getPointerSizeInBits(0));
   const LLT s1 = LLT::scalar(1);
   const LLT s8 = LLT::scalar(8);
   const LLT s16 = LLT::scalar(16);
   const LLT s32 = LLT::scalar(32);
   const LLT s64 = LLT::scalar(64);
   const LLT s128 = LLT::scalar(128);
 
   for (auto Ty : {p0, s1, s8, s16, s32})
void X86LegalizerInfo::setLegalizerInfo32bit() {
   }
 }
 
 void X86LegalizerInfo::setLegalizerInfo64bit() {
 
   if (!Subtarget.is64Bit())
     return;
 
-  const LLT p0 = LLT::pointer(0, TM.getPointerSize() * 8);
+  const LLT p0 = LLT::pointer(0, TM.getPointerSizeInBits(0));
   const LLT s1 = LLT::scalar(1);
   const LLT s8 = LLT::scalar(8);
   const LLT s16 = LLT::scalar(16);
   const LLT s32 = LLT::scalar(32);
   const LLT s64 = LLT::scalar(64);
   const LLT s128 = LLT::scalar(128);
 
   setAction({G_IMPLICIT_DEF, s64}, Legal);