This is an archive of the discontinued LLVM Phabricator instance.

[IC] Turn non-null MD on pointer loads to range MD on integer loads.
ClosedPublic

Authored by cdavis5x on Feb 13 2015, 12:19 PM.

Details

Summary

This change fixes the FIXME that you recently added when you committed
(a modified version of) my patch. When InstCombine combines a load and
store of an pointer to those of an equivalently-sized integer, it currently
drops any !nonnull metadata that might be present. This change replaces
!nonnull metadata with !range !{ 1, -1 } metadata instead.

(Yes, everything with my Gmail account is OK, now. Thanks for your help!)

Diff Detail

Repository
rL LLVM

Event Timeline

cdavis5x updated this revision to Diff 19919.Feb 13 2015, 12:19 PM
cdavis5x retitled this revision from to [IC] Turn non-null MD on pointer loads to range MD on integer loads..
cdavis5x updated this object.
cdavis5x edited the test plan for this revision. (Show Details)
cdavis5x added a reviewer: chandlerc.
cdavis5x added a subscriber: Unknown Object (MLST).
reames added a subscriber: reames.Feb 18 2015, 10:33 AM
reames added inline comments.
lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
340 ↗(On Diff #19919)

For consistency, I'd prefer to see this as:
else if (NewTy->isIntegerTy()) {

auto *ITy = cast<IntegerType>(NewTy);
343 ↗(On Diff #19919)

Meta comment: This does assume that null is integer value 0. I think that's in practice a safe assumption.

345 ↗(On Diff #19919)

I think you'd be better off using MDBuilder::createRange here.

test/Transforms/InstCombine/loadstore-metadata.ll
88 ↗(On Diff #19919)

You need more specific tests here. in particular, what does the metadata look like?. Other cases to consider: store(p, load(p)), bitcast(load(p), IntTy),

cdavis5x added inline comments.Feb 20 2015, 12:15 PM
lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
343 ↗(On Diff #19919)

You know, the possibility of null != 0 crossed my mind, but I dismissed it because like you said, in practice, on every platform we support, null == 0. In fact, the only example I can think of off the top of my head is segmented-memory-model x86, where null would be a 0 segment and an undef offset (i.e. doesn't matter)--and I get the feeling that nobody around here wants to support that. ;)

Do you want me to add a FIXME or some other comment here noting this?

test/Transforms/InstCombine/loadstore-metadata.ll
88 ↗(On Diff #19919)

I'm pretty sure InstCombine combines a store of a load from the same pointer away.

Strangely, InstCombine doesn't convert your second case (which really should be ptrtoint(load(p), IntTy), but I digress) to load(bitcast(p, IntTy*)). That might be something to look into.

cdavis5x updated this revision to Diff 20424.Feb 20 2015, 12:15 PM

Incorporate @reames' suggested changes.

reames added inline comments.Feb 23 2015, 11:34 AM
lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
343 ↗(On Diff #19919)

No comment needed. This is a pretty wide spread issue.

LGTM, though please hold off on the submit until Chandler has a chance to glance over it. He knows this code better than I do.

lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
346 ↗(On Diff #20424)

I'd have a mild preference that the builder be pulled out into it's own statement, but we're into nits at this point. Feel free to ignore.

chandlerc accepted this revision.Feb 23 2015, 12:26 PM
chandlerc edited edge metadata.

Good to commit with the MDBuilder hoisted and my question about the metadata CHECK line addressed.

lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
346 ↗(On Diff #20424)

Yea, it'd be good to create the MDBuilder above, outside of the loop, etc.

351–353 ↗(On Diff #20424)

Do you want to handle the inverse in this patch? That would then make for a nice round-trip test that we can go from pointer -> int -> pointer and preserve non-null.

340 ↗(On Diff #19919)

Can use continue here to avoid the else.

test/Transforms/InstCombine/loadstore-metadata.ll
98 ↗(On Diff #20424)

Does this work even if the above test case isn't the last one in the file? I wonder if we need to make the IR printing denormalize this somewhat for easier checking.

This revision is now accepted and ready to land.Feb 23 2015, 12:26 PM
cdavis5x added inline comments.Feb 23 2015, 1:48 PM
lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
351–353 ↗(On Diff #20424)

That would be nice, but I think I'd like to be conservative in this patch for now. I'll see about turning !range MD into !nonnull MD later.

Besides, this raises some questions of its own, like: if we're converting an integer load to a pointer load, and the integer load has a !range other than !{iPTR 1, iPTR -1} (but that still doesn't include 0, or whatever ptrtoint null is), should we then convert the !range to !nonnull MD? (I think we should, but the problem is that we lose some information there. I suspect this is part of why you held off on that. :)

test/Transforms/InstCombine/loadstore-metadata.ll
98 ↗(On Diff #20424)

Probably not, given that it's a plain ol' CHECK line. If any more CHECK lines are added after this, they will fail, because FileCheck will have already scanned past them.

Frankly, I was kind of worried about that when I wrote this. (I should've brought it up then...) I considered using CHECK-DAG, but I think that only works between the previous and next CHECK-LABEL lines. We might need to do what you said and have the IR pretty printer emit MD tuples immediately after the function that uses them (for instance) rather than always at the end of the file.

cdavis5x updated this revision to Diff 20559.Feb 23 2015, 5:33 PM
cdavis5x edited edge metadata.

Incorporate @reames' and @chandlerc's recommendations. In particular:

  • Hoist the MDBuilder object out of the loop.
  • Break early in the pointer type case; this eliminates an else.

Also:

  • Add a FIXME for !range -> `!nonnull' MD.
chandlerc added inline comments.Feb 23 2015, 5:45 PM
lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
351–353 ↗(On Diff #20424)

This really raises a whole host of problems.

Is it even correct to translate !nonnull -> an integer range that doesn't include zero? Do we guarantee that ptrtoint always produces a zero integer for a null pointer? I don't think we do. And in practice, I don't think that is always safe.

I'm starting to think this entire endeavor is not safe, and we need to take several steps back.

cdavis5x added inline comments.Feb 23 2015, 5:53 PM
lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
351–353 ↗(On Diff #20424)

All right. Want me to just abandon this for now?

chandlerc added inline comments.Feb 23 2015, 6:05 PM
lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
351–353 ↗(On Diff #20424)

No, we've been debating thin in the IRC channel.

We think this is all "ok". What we should do is compute the range by computing constantexprs with the null pointer and ptrtoint. Essentially, define the range as ["(add (ptrtoint null) 1)", "(ptrtoint null)"). And we can use the same address space of null pointer for the ptrtoint, and this code will be no more or less wrong than the constant folder.

For the handling the reverse case, we just check whether (ptrtoint null) is outside of the range, and if so, synthesize !nonnull.

test/Transforms/InstCombine/loadstore-metadata.ll
98 ↗(On Diff #20424)

Are you planning to look into that?

Either way, can you leave a comment here to help the next person to debug this?

cdavis5x updated this revision to Diff 20616.Feb 24 2015, 12:25 PM

Use ConstantExprs instead of assuming ptrtoint null is 0.

Also:

  • The integer range is now (as @chandlerc suggested) !{add (ptrtoint null, 1), ptrtoint null}--under normal circumstances, that should be !{1, 0} instead of !{1, -1}.
  • Add a big scary FIXME warning others not to add CHECK-LABEL lines after the !nonnull -> !range test.
  • Add an MDBuilder::createRange overload to build range MD tuples from Constants. (Hope that's OK; I needed it for this patch, because it's difficult to get APInts out of ConstantExprs.)
  • Fix some syntax errors I let out the last time. (That'll teach me to make last minute changes right before updating without building... ;)
cdavis5x added inline comments.Feb 24 2015, 12:29 PM
test/Transforms/InstCombine/loadstore-metadata.ll
98 ↗(On Diff #20424)

This doesn't seem to be pressing. Perhaps later.

I've left a FIXME in the code for now.

LGTM! Thanks for working on this!

This revision was automatically updated to reflect the committed changes.