Page MenuHomePhabricator

NFC. Move getAlignment helper function from ValueTracking to Value class.
ClosedPublic

Authored by apilipenko on Jan 13 2016, 6:28 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

apilipenko updated this revision to Diff 44748.Jan 13 2016, 6:28 AM
apilipenko retitled this revision from to NFC. Move getAlignment helper function from ValueTracking to Value class..
apilipenko updated this object.
apilipenko added reviewers: hfinkel, reames.
apilipenko added a subscriber: llvm-commits.
reames edited edge metadata.Jan 13 2016, 12:24 PM

Why do you want to move this? It doesn't seem like an obvious place for this. Not saying no, just trying to understand the motivation.

include/llvm/IR/Value.h
501 ↗(On Diff #44748)

Documentation?

I'm going to move isDereferenceableAndAligned stuff from ValueTracking to Loads as Hal suggested in D10920. This functionality uses getAlignment static function which is also used by computeKnownBits machinery.

Why to Value class instead of making it a public ValueTracking function? It queries inherent properties of a Value just like isDereferenceable/getDereferenceableBytes I've introduced in D16116. All the three functions must be placed somewhere together, however I don't have strong preference about Value class being this place.

Given this code sometimes gets the ABI alignment and sometimes gets the preferred alignment, I'm not really clear on what the predicate actually is. Unless you can write a clear bit of documentation which makes it obvious, I don't think this makes sense to promote to Value.

include/llvm/IR/Value.h
505 ↗(On Diff #44748)

It seems like this change might be based on something other than ToT. I can't find this function in IR/Value.h in ToT. Nor do I see it in the single listed dependency.

Can this be a public function in ValueTracking?

apilipenko added inline comments.Jan 15 2016, 9:50 AM
include/llvm/IR/Value.h
505 ↗(On Diff #44748)

Yes, this was based on D16116. However I didn't put it into dependency list, because there is no actual dependence.

hfinkel added inline comments.Feb 2 2016, 6:25 PM
include/llvm/IR/Value.h
501 ↗(On Diff #44748)

This change makes sense to me, but as Phlip says, the function needs documentation to be in a public header.

apilipenko updated this revision to Diff 47201.Feb 8 2016, 7:51 AM
apilipenko edited edge metadata.

Add getPointerAlignment documentation.

reames accepted this revision.Feb 22 2016, 2:10 PM
reames edited edge metadata.

LGTM w/very minor comment.

lib/IR/Value.cpp
543 ↗(On Diff #47201)

This ternary constant folds.

This revision is now accepted and ready to land.Feb 22 2016, 2:10 PM
This revision was automatically updated to reflect the committed changes.