This is an archive of the discontinued LLVM Phabricator instance.

IR: Change PointerType to derive from Type rather than SequentialType.
ClosedPublic

Authored by pcc on Nov 13 2016, 1:24 PM.

Details

Summary

As proposed on llvm-dev:
http://lists.llvm.org/pipermail/llvm-dev/2016-October/106640.html

This is for a couple of reasons:

  • Values of type PointerType are unlike the other SequentialTypes (arrays and vectors) in that they do not hold values of the element type. By moving PointerType we can unify certain aspects of how the other SequentialTypes are handled.
  • PointerType will have no place in the SequentialType hierarchy once pointee types are removed, so this is a necessary step towards removing pointee types.

Depends on D26594

Event Timeline

pcc updated this revision to Diff 77757.Nov 13 2016, 1:24 PM
pcc retitled this revision from to IR: Change PointerType to derive from Type rather than SequentialType..
pcc updated this object.
pcc added a reviewer: dblaikie.
pcc added a subscriber: llvm-commits.
dblaikie added inline comments.Nov 21 2016, 2:59 PM
llvm/include/llvm/IR/DerivedTypes.h
316

Might be best to save the refactoring here for a separate patch - make things a bit easier to follow (ie: which bit makes PointerType not a SequentialType, and then which things are improvements now that all SequentialTypes have numElements, etc).

Maybe. Open to debate/discussion/disagreement for sure.

454–457

This should make it much easier for me to test how the typeless pointer work is progressing (previously I had to do a dynamic check in SequentialType::getElementType (assert(!isa<PointerType>) basically). Handy :)

pcc updated this revision to Diff 79272.Nov 24 2016, 9:11 PM

Split out the getNumElements() change

pcc marked an inline comment as done.Nov 24 2016, 9:12 PM
pcc added inline comments.
llvm/include/llvm/IR/DerivedTypes.h
316

Seems reasonable enough. Done: : D27122

dblaikie edited edge metadata.Nov 29 2016, 10:29 AM

Looks generally good - I just don't understand how some of the changes are valid. Would be helpful to know.

llvm/lib/IR/ConstantFold.cpp
2208–2212

Is there a guarantee that it's never really a pointer type here?

llvm/lib/Transforms/IPO/ArgumentPromotion.cpp
178–179

This looks like it changes the behavior of the code.

Previously the code returned here if type was a PointerType - now it doesn't return, but continues into the rest of the function.

Is this just because type was never really a PointerType? (perhaps you could add an assert to that effect)

llvm/lib/Transforms/Scalar/SROA.cpp
3215–3219

This also appears to change the behavior - if Ty is a PointerType this code previously returned, now it skips this block and continues. Is this correct? Because Ty is never a PointerType here (add an assert?), or because the following code doesn't do anything if it is? (maybe still worth an early return defensively - oh, I see, after this block it returns if non-struct, so that's pretty obvious & maybe doesn't need anything extra)

pcc marked an inline comment as done.Nov 29 2016, 12:35 PM
pcc added inline comments.
llvm/lib/IR/ConstantFold.cpp
2208–2212

The purpose of this loop is to look through a notional GEP's element types. Earlier we initialized Ty to PointeeTy which, at the only call site [0], is the notional GEP's source element type. Because the source element type cannot be a pointer, and the same goes for later element types, having a pointer type here is impossible and this was dead code before.

[0] http://llvm-cs.pcc.me.uk/lib/IR/Constants.cpp#1906

llvm/lib/Transforms/IPO/ArgumentPromotion.cpp
178–179

Now that PointerTypes are no longer CompositeTypes, the return happens on line 175 above.

llvm/lib/Transforms/Scalar/SROA.cpp
3215–3219

Yes, I think the existing code is sufficient.

pcc added inline comments.Nov 29 2016, 1:07 PM
llvm/lib/IR/ConstantFold.cpp
2208–2212

Correction: the loop only looks at indexable element types. Of course the source element type could be a pointer (or any other type) if the GEP has only one operand, and the target element type may be non-indexable regardless of the number of operands. But this loop has (operands-1) iterations (see line 2189 above), which means that we never look at the target element type.

dblaikie accepted this revision.Dec 1 2016, 2:41 PM
dblaikie edited edge metadata.
This revision is now accepted and ready to land.Dec 1 2016, 2:41 PM

Looks like Phab might not've sent email - anyway, signed off on this. Carry on :)

This revision was automatically updated to reflect the committed changes.