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

[Tim Northover via Phabricator via llvm-commits]

t.p.northover marked 2 inline comments as done.
t.p.northover 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));
   }
----------------
arsenm wrote:
> 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
This isn't directly a size query though. It's related to `getMemoryVT` on a truncating store or other `MemSDNode`, or at least that's how I view it. And that type is pretty much independent of any alignment requirements.

But even without that, I still don't think we can change it. It would alter existing behaviour on any target with non-natural pointer alignment.

The best we could do is rename the function so that the confusion doesn't occur, `getPointerStoreTy` perhaps?


================
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:
> 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
Very good idea. I'll do that.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D58982