This is an archive of the discontinued LLVM Phabricator instance.

Fix invalid ptrtoint in InstCombine
ClosedPublic

Authored by yuyichao on Jun 18 2017, 3:09 PM.

Details

Summary

A few things I'm not sure about.

  1. Is this the right place to do it? (It seems that this is used in both instcombine and jumpthreading with a similar pattern but I'm not sure if this can be triggerred in jumpthreading)
  2. Do I need the address space check? I can't find a different place that checks the address space matching and the caller does not try addrspacecast (and probably shouldn't) though I also can't trigger the code with two address space ....

Diff Detail

Event Timeline

yuyichao created this revision.Jun 18 2017, 3:09 PM
loladiro added inline comments.Jun 29 2017, 12:33 PM
lib/Analysis/Loads.cpp
373

I'm a bit confused by this. Isn't Ptr the thing we're loading from? Why are we comparing it against LI->getType() shouldn't we be looking at AccessTy?

yuyichao updated this revision to Diff 108805.Jul 29 2017, 2:43 PM
yuyichao updated this revision to Diff 108806.Jul 29 2017, 2:47 PM

Use AccessTy and added a test to make sure that the case where we load from an non-integral point is still optimized.

majnemer added inline comments.Sep 26 2017, 8:22 PM
lib/Analysis/Loads.cpp
375–385

Please fix the formatting.

407–418

Ditto.

majnemer edited edge metadata.Sep 26 2017, 8:24 PM

Your code has no comments. At a glance, I have no idea what it is trying to accomplish.

yuyichao updated this revision to Diff 116807.Sep 27 2017, 7:22 AM

There was no comment since the only comment I can add would be repeating the condition itself. I do like to add comment about why this is needed or if this is the only way we can fix the issue but that's also what I'm not sure about (quesiton 1 in my review comment). Anyway, I added a comment to repeat the condition for now.

yuyichao updated this revision to Diff 118306.Oct 9 2017, 7:38 PM

I've found another failure in instcombine (see last test) so it seems to me that CastInst::isBitOrNoopPointerCastable is the right place for this logic. This also answers my second question since the cast between address spaces is already handled by isBitCastable

sanjoy accepted this revision.Oct 21 2017, 10:41 PM

LGTM

This revision is now accepted and ready to land.Oct 21 2017, 10:41 PM

Thanks for the review.

This revision was automatically updated to reflect the committed changes.