[PATCH] D58982: DAG: allow DAG pointer size different from memory representation.

[Tim Northover via Phabricator via llvm-commits]

t.p.northover marked 3 inline comments as done.
t.p.northover added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/Analysis.h:76-81
+/// Variant of ComputeValueVTs that also produces the memory VTs.
+void ComputeValueVTs(const TargetLowering &TLI, const DataLayout &DL, Type *Ty,
+                     SmallVectorImpl<EVT> &ValueVTs,
+                     SmallVectorImpl<EVT> *MemVTs,
+                     SmallVectorImpl<uint64_t> *Offsets = nullptr,
+                     uint64_t StartingOffset = 0);
----------------
arsenm wrote:
> Can you just consolidate this into one version?
Yes, but that would come with different costs. Either the MemVTs argument would move to the end, which seemed inconsistent to me, or there would be significantly more churn for uses that specify one of the defaults.

I'd probably favour taking the churn hit, and would be happy to implement it if you want.


================
Comment at: llvm/include/llvm/CodeGen/TargetLowering.h:245
+  MVT getPointerMemTy(const DataLayout &DL, uint32_t AS = 0) const {
     return MVT::getIntegerVT(DL.getPointerSizeInBits(AS));
   }
----------------
arsenm wrote:
> Shouldn't this be the ABI size in bits? At one point we were using 32-bit pointers with 64-bit ABI alignment for something similar
I don't think so. As used, its most critical property is that it should be the same as getPointerTy for the bulk of targets that don't want to go fiddling with this.

I suppose there is some risk that people will mistake it for including alignment padding, but I think I'd expect all bits to be meaningful in a function named to get a type like this, and a separate interface for alignment if needed.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:1625-1626
+      SDValue Val = RetOp.getValue(i);
+      if (MemVTs[i] != ValueVTs[i])
+        Val = DAG.getZExtOrTrunc(Val, getCurSDLoc(), MemVTs[i]);
+      Chains[i] = DAG.getStore(Chain, getCurSDLoc(), Val,
----------------
arsenm wrote:
> It seems to me that this is target specific to assume zext
Yes, we've had one or two teams request a notionally signed pointer so that they can use ILP32 in their kernel (AArch64 OSs live in very high memory).

I'm not too keen on supporting parametrization like that from the start though. To begin with it would be completely untested for the foreseeable future.

I also have some slight reservations about the semantics. Nothing fatal, but questions of objects contiguous across 0 and pointer differences seem like something you'd want to resolve when you've got a target in front of you to experiment and test on.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58982/new/

https://reviews.llvm.org/D58982