Page MenuHomePhabricator

[opaque pointer types] [NFC] GEP: replace get(Pointer)ElementType uses with get{Source,Result}ElementType.
ClosedPublic

Authored by eddyb on Jan 17 2016, 12:46 PM.

Details

Summary

GEPOperator: provide getResultElementType alongside getSourceElementType.
This is made possible by adding a result element type field to GetElementPtrConstantExpr, which GetElementPtrInst already has.

GEP: replace get(Pointer)ElementType uses with get{Source,Result}ElementType.

Diff Detail

Repository
rL LLVM

Event Timeline

eddyb updated this revision to Diff 45114.Jan 17 2016, 12:46 PM
eddyb retitled this revision from to [opaque pointer types] GEP: replace get(Pointer)ElementType uses with get{Source,Result}ElementType..
eddyb updated this object.
eddyb added a reviewer: mjacob.
eddyb added subscribers: dblaikie, llvm-commits.
eddyb retitled this revision from [opaque pointer types] GEP: replace get(Pointer)ElementType uses with get{Source,Result}ElementType. to [opaque pointer types] [NFC] GEP: replace get(Pointer)ElementType uses with get{Source,Result}ElementType..Jan 17 2016, 12:57 PM
eddyb updated this revision to Diff 45116.Jan 17 2016, 1:03 PM

Fixed missing PtrTy in ScalarReplAggregates.cpp's CanConvertToScalar.

eddyb removed a subscriber: dblaikie.
eddyb updated this revision to Diff 45190.Jan 18 2016, 8:52 AM

Simplified the first commit and took out the second one to shorten this revision.

eddyb updated this object.Jan 18 2016, 8:53 AM
eddyb updated this revision to Diff 45198.Jan 18 2016, 11:18 AM

Remove change that depends on other patches (and does nullptr deref right now).

mjacob added inline comments.Jan 18 2016, 1:29 PM
lib/Analysis/ScalarEvolution.cpp
4090 ↗(On Diff #45190)

You could move this variable closer to its only consumer or even inline it.

lib/CodeGen/SelectionDAG/FastISel.cpp
517 ↗(On Diff #45198)

Please add a comment why this is correct here, i.e. that Ty can be a pointer type only on the first iteration.

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
3022 ↗(On Diff #45198)

Same here.

lib/IR/ConstantFold.cpp
2045 ↗(On Diff #45198)

Maybe this code was wrong, because Ptr->getScalarType() returns Ptr in any case. However I couldn't find a way to trigger a bug. I'll investigate this. Your changes look OK.

lib/IR/ConstantsContext.h
228 ↗(On Diff #45198)

You should mention in the commit message that you add a field to this class.

lib/Target/AArch64/AArch64FastISel.cpp
4829 ↗(On Diff #45198)

Please add a comment - same as above.

lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
697–698 ↗(On Diff #45198)

Can we use getResultElementType() here?

lib/Transforms/InstCombine/InstructionCombining.cpp
1421 ↗(On Diff #45198)

I think CurTy is implied by J > 1. IMHO the extra check creates confusion.

eddyb marked an inline comment as done.Jan 18 2016, 2:50 PM
eddyb added inline comments.
lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
697–698 ↗(On Diff #45198)

If Ops were to have all of GEPI's indices, then if (Idx == GEPI->getNumOperands()) return false; above would've handled it (by returning out of this function).
In other words, AllocTy will always stop short of the ResultElementType, as less indices will be applied.

eddyb updated this object.Jan 18 2016, 4:08 PM
eddyb updated this revision to Diff 45218.Jan 18 2016, 4:14 PM
eddyb marked 7 inline comments as done.

Address review comments.

eddyb updated this revision to Diff 45270.Jan 19 2016, 8:56 AM

Rebased on top of rL258134.

mjacob accepted this revision.Jan 19 2016, 9:19 AM
mjacob edited edge metadata.

LGTM.

lib/Analysis/ValueTracking.cpp
3255 ↗(On Diff #45270)

I think this variable should be inlined to its only user.

This revision is now accepted and ready to land.Jan 19 2016, 9:19 AM
This revision was automatically updated to reflect the committed changes.
eddyb marked an inline comment as done.
dblaikie added inline comments.Jan 19 2016, 10:46 AM
llvm/trunk/lib/CodeGen/SelectionDAG/FastISel.cpp
516

This seems like a slightly indirect test of the property you describe in the comment.

Do you think it might be nicer to check if the iterator is == I->op_begin() + 1? Or to add a flag of some kind before the loop for this first case? Or perhaps to use a gep type iterator in parallel with the operands? (or an assert for any of these conditions)

Not necessary, just throwing some ideas around in case any of them seem like an improvement.

llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
3021

Same as above

llvm/trunk/lib/Target/AArch64/AArch64FastISel.cpp
4828

Same as above

(starting to see a pattern - perhaps there's a common API/abstraction we could use for these?)

eddyb added inline comments.Jan 19 2016, 3:34 PM
llvm/trunk/lib/CodeGen/SelectionDAG/FastISel.cpp
516

I think I would use gep_type_iterator, but I would have to rewrite each of these loops (and maybe make gep_type_iterator nicer to use, with C++11 range-for sugar perhaps).

Although, now that I've gotten myself set with a fast rebase/rebuild cycle, it shouldn't be such an ordeal anymore, I might just try it out.