Page MenuHomePhabricator

[opaque pointer types] [NFC] Take advantage of get{Source,Result}ElementType when folding GEPs.
ClosedPublic

Authored by eddyb on Jan 18 2016, 12:50 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

eddyb updated this revision to Diff 45204.Jan 18 2016, 12:50 PM
eddyb retitled this revision from to [opaque pointer types] [NFC] ConstantFoldInstOperands: add a form taking the Instruction or ConstantExpr for GEPs..
eddyb updated this object.
eddyb added reviewers: mjacob, dblaikie.
eddyb added a subscriber: llvm-commits.
eddyb updated this revision to Diff 45275.Jan 19 2016, 9:35 AM

Rebased on top of rL258145.

eddyb updated this object.Jan 19 2016, 9:37 AM
mjacob edited edge metadata.Jan 19 2016, 10:24 AM

I'm stopping reviewing here. Please create a separate patch for the changes in SymbolicallyEvaluateGEP.

include/llvm/Analysis/ConstantFolding.h
50–55 ↗(On Diff #45204)

Instead of introducing this function with the implicit requirement of having to provide InstOrCE if Opcode == Instruction::GetElementPtr, this should be made explicit by introducing a function specifically for GEPs (similar to ConstantFoldCompareInstOperands()).

lib/Analysis/ConstantFolding.cpp
729–730 ↗(On Diff #45204)

The lack of consistency of parameter names is slightly confusing here. Am I right that ResultTy is the pointer type of the GEP, and SrcTy and ResultElementTy are pointee types?

734 ↗(On Diff #45204)

Ptr is always a constant of pointer type, right? I'll remove this check in a separate commit.

eddyb added inline comments.Jan 19 2016, 4:29 PM
include/llvm/Analysis/ConstantFolding.h
50–55 ↗(On Diff #45275)

That's not really possible without introducing a special case at each call site.
Or maybe I can add a function to wrap ConstantFoldInstOperands(I, I->getOpcode(), I->getType(), ...) in the Instruction* case, that seems more plausible.

mjacob added inline comments.Jan 20 2016, 5:53 PM
include/llvm/Analysis/ConstantFolding.h
50–55 ↗(On Diff #45275)

Thinking over this again, I don't like what I proposed in my last comment. Instead, I created the revisions D16378, D16380, D16383 to refactor the API to make this change a bit easier.

eddyb updated this revision to Diff 45486.Jan 20 2016, 8:26 PM
eddyb edited edge metadata.

Rebased on top D16383.

eddyb retitled this revision from [opaque pointer types] [NFC] ConstantFoldInstOperands: add a form taking the Instruction or ConstantExpr for GEPs. to [opaque pointer types] [NFC] Take advantage of get{Source,Result}ElementType when folding GEPs..Jan 20 2016, 8:26 PM
eddyb updated this revision to Diff 45561.Jan 21 2016, 10:41 AM

Moved the invocation of SymbolicallyEvaluateGEP and its StructType handling to simplify the patch.

dblaikie added inline comments.Jan 21 2016, 3:20 PM
lib/Analysis/ConstantFolding.cpp
739 ↗(On Diff #45561)

How/why did this get rolled in here?

mjacob accepted this revision.Jan 21 2016, 3:24 PM
mjacob edited edge metadata.

LGTM

lib/Analysis/ConstantFolding.cpp
743–744 ↗(On Diff #45561)

Actually Ptr can also be a vector of pointers here. I have reverted the commit in which I moved this check to an assertion. When merging / rebasing, make sure to include the isPointerTy() check, but *not* the assertion.

This revision is now accepted and ready to land.Jan 21 2016, 3:24 PM
eddyb added inline comments.Jan 21 2016, 3:35 PM
lib/Analysis/ConstantFolding.cpp
739 ↗(On Diff #45561)

I had two calls to SymbolicallyEvaluateGEP at some point and it was easier to have it here than two CastGEPIndices at each site.

IMO CastGEPIndices shouldn't bother creating a GEP ConstantExpression but rather change what Ops in this function points to (if any indices were cast).

This revision was automatically updated to reflect the committed changes.
dblaikie added inline comments.Jan 21 2016, 4:24 PM
lib/Analysis/ConstantFolding.cpp
739 ↗(On Diff #45561)

Realize that, to speed throughput, I'm looking at these changes /really/ superficially/mechanically. Anything that's more than a very local change for accessing type information slows me down a fair bit & I may not understand it fully. (case in point here - I haven't looked at what this function is trying to do or how it might best do that - just at whether the behavior seems to be preserved by moving around the type information and how it's accessed) :)

So the more you can help me understand anything more complicated than that (or avoid such complications/separate them into pre or post patches), the better.

860 ↗(On Diff #45561)

Was this dead code previously?