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

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
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));
   }
----------------
t.p.northover wrote:
> 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.
That should end up being the same on every target? Everywhere else the "memory size" always includes the alignment padding


================
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,
----------------
t.p.northover wrote:
> 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.
I would prefer to document then with a getPtrExtOrTrunc, which will just be an alias


Repository:
  rL LLVM

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

https://reviews.llvm.org/D58982