Page MenuHomePhabricator

DAG: allow DAG pointer size different from memory representation.
ClosedPublic

Authored by t.p.northover on Mar 5 2019, 10:16 AM.

Details

Reviewers
arsenm
Summary

In preparation for supporting ILP32 on AArch64, this modifies the SelectionDAG builder code so that pointers are allowed to have a larger type when "live" in the DAG compared to memory.

Pointers get zero-extended whenever they are loaded, and truncated prior to stores. In addition, a few not quite so obvious locations need updating:

  • A GEP that has not been marked inbounds needs to enforce the IR-documented 2s-complement wrapping at the memory pointer size. Inbounds GEPs are undefined if they overflow the address space, so no additional operations are needed.
  • Signed comparisons would give incorrect results if performed on the zero-extended values.

This shouldn't affect CodeGen for now, which unfortunately means tests can't be written, but will become active when the AArch64 ILP32 support is committed. I decided it was better to split the patch off despite that limitation.

Diff Detail

Repository
rL LLVM

Event Timeline

t.p.northover created this revision.Mar 5 2019, 10:16 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 5 2019, 10:16 AM
arsenm added a subscriber: arsenm.Apr 15 2019, 4:04 AM
arsenm added inline comments.
llvm/include/llvm/CodeGen/Analysis.h
76–81

Can you just consolidate this into one version?

llvm/include/llvm/CodeGen/TargetLowering.h
245

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

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
1625–1626

It seems to me that this is target specific to assume zext

t.p.northover marked 3 inline comments as done.Apr 15 2019, 4:35 AM
t.p.northover added inline comments.
llvm/include/llvm/CodeGen/Analysis.h
76–81

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.

llvm/include/llvm/CodeGen/TargetLowering.h
245

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.

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
1625–1626

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.

arsenm added inline comments.Apr 15 2019, 1:46 PM
llvm/include/llvm/CodeGen/TargetLowering.h
245

That should end up being the same on every target? Everywhere else the "memory size" always includes the alignment padding

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
1625–1626

I would prefer to document then with a getPtrExtOrTrunc, which will just be an alias

t.p.northover marked 2 inline comments as done.Apr 16 2019, 1:50 AM
t.p.northover added inline comments.
llvm/include/llvm/CodeGen/TargetLowering.h
245

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?

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
1625–1626

Very good idea. I'll do that.

Switched to using getPtrExtOrTrunc. I *think* it's right, but there was a certain amount of guesswork on whether it'll actually work for signed pointers.

I haven't changed ComputeValueVTs yet.

I haven't changed ComputeValueVTs yet.

And that's because it looks like discussion is ongoing rather than because I'll be doing that and uploading soon.

arsenm accepted this revision.Mon, Apr 29, 2:19 PM

LGTM

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
2337

This reads backwards to me

3228

Ditto

This revision is now accepted and ready to land.Mon, Apr 29, 2:19 PM
t.p.northover closed this revision.Wed, May 1, 5:35 AM

Thanks Matt. Committed as r359676 with your suggested boolean flip.

sanjoy added a subscriber: sanjoy.Wed, May 8, 7:36 PM
sanjoy added inline comments.
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
4711

This line crashes when we have an atomic store of a floating point number. Simple reproducer:

target datalayout = "e-i64:64-i128:128-v16:16-v32:32-n16:32:64"
target triple = "nvptx64-nvidia-cuda"

define void @f(float %a, float* %b) {
entry:
  store atomic float %a, float* %b unordered, align 4
  ret void
}

I think it needs to bitcast the value operand to an integer before truncating or extending it.

t.p.northover marked an inline comment as done.Thu, May 9, 2:46 AM
t.p.northover added inline comments.
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
4711

Sorry about that. I've put up a review to fix it in https://reviews.llvm.org/D61721 (the types are only different for pointers, so no need to add any extra casts).

I think it's pretty trivial, but just wanted to make sure I wasn't breaking some NVPTX testing rules or anything.