This is an archive of the discontinued LLVM Phabricator instance.

Reduce dependence on pointee types when deducing dereferenceability
ClosedPublic

Authored by sanjoy on May 27 2016, 6:50 PM.

Details

Summary

Change some of the internal interfaces in Loads.cpp to keep track of the
number of bytes we're trying to prove dereferenceable using an explicit
Size parameter.

Before this, the Size parameter was implicitly inferred from the
pointee type of the pointer whose dereferenceability we were trying to
prove, causing us to be conservative around bitcasts. This was
unfortunate since bitcast instructions are no-ops and should never
break optimizations. With an explicit Size parameter, we're more
precise (as shown in the test cases), and the code is simpler.

We should eventually move towards a DerefQuery struct that groups
together a base pointer, an offset, a size and an alignment; but this
patch is a first step.

Diff Detail

Event Timeline

sanjoy updated this revision to Diff 58875.May 27 2016, 6:50 PM
sanjoy retitled this revision from to Reduce dependence on pointee types when deducing dereferenceability.
sanjoy updated this object.
sanjoy added reviewers: apilipenko, dblaikie, hfinkel, reames.
sanjoy added a subscriber: llvm-commits.
apilipenko added inline comments.May 30 2016, 2:40 AM
lib/Analysis/Loads.cpp
145–146

You change the behavior for opaque types here. Is it intentional? Previous version tried to give an answer for the cases where it could guarantee dereferenceability, e.g. load from an opaque typed global. I thought that we even have a test case for that.

If we don't want to support this case anymore, the code can be simplified further. Value::isPointerDereferenceable can be removed, instead we can rely on getPointerDereferenceable to return dereferenceable bytes for the cases now handled by isPointerDereferenceable.

If we do want to support both sized and opaque types it might make sense to separate handling into two functions, one for sized one for non-sized.

sanjoy added inline comments.May 30 2016, 1:19 PM
lib/Analysis/Loads.cpp
145–146

This wasn't intentional, but I also don't remember any upstream llvm-lit tests failing (which is why I thought the change was harmless). I'll take a closer look tomorrow.

sanjoy marked an inline comment as done.Jun 1 2016, 9:32 AM
sanjoy added inline comments.
lib/Analysis/Loads.cpp
145–146

Hi Artur,

I've "fixed" this issue by defining unsized loads and stores as illegal in http://reviews.llvm.org/rL271402

apilipenko accepted this revision.Jun 1 2016, 9:38 AM
apilipenko edited edge metadata.

LGTM. As I mentioned, since we don't support opaque types here, we don't need isPointerDereferenceable. Corresponding logic should be moved to getPointerDereferenceableBytes.

This revision is now accepted and ready to land.Jun 1 2016, 9:38 AM
This revision was automatically updated to reflect the committed changes.