Page MenuHomePhabricator

[InstCombine] Fix inbounds gep for addrspacecasts
Needs ReviewPublic

Authored by svenvh on May 19 2017, 9:46 AM.

Details

Reviewers
majnemer
Summary

The inbounds inference added by r277950 ("[InstCombine] Infer inbounds
on geps of allocas", 2016-08-07) could construct a new inbounds gep,
but ignore the address space.

Avoid asserting when the address space does not match. We could
introduce a new addrspacecast, but as one of the reviewers pointed
out, it is not clear that instcombine is allowed to introduce
addrspacecasts that weren't in the original program.

Diff Detail

Event Timeline

svenvh created this revision.May 19 2017, 9:46 AM

Please see the discussion in D31924 - it's not clear that instcombine is allowed to introduce addrspacecasts that weren't in the original program. I know there's a couple cases like that in the code base already, but no need to add more.

sanjoy added a subscriber: sanjoy.May 21 2017, 10:20 PM
svenvh added a subscriber: arsenm.May 22 2017, 1:56 AM

It seems commit r215467 ("Allwo bitcast + struct GEP transform to work with addrspacecast", 2014-08-12) already assumes it can swap GEPs and addrspacecasts indeed. Adding @arsenm; perhaps we should reconsider that commit?

In the meantime, I can update this patch to simply not infer inbounds when address spaces don't match, so that at least we no longer assert on the test case. Would that make sense?

tauril added a subscriber: tauril.Mar 29 2018, 7:59 AM

Hello, I've recently come across the same problem as expressed by this revision.
Has there been any new discussions regarding this topic?

I've attached a small sample that triggers the type checking assertion in replaceAllUsesWith.

svenvh added a comment.Apr 3 2018, 6:32 AM

Hello, I've recently come across the same problem as expressed by this revision.
Has there been any new discussions regarding this topic?

Thanks for pinging this, Guillaume. I don't think we have reached a conclusion here, so I'll rework the patch to just avoid inferring inbounds for these cases.

svenvh updated this revision to Diff 140981.Apr 4 2018, 9:48 AM
svenvh edited the summary of this revision. (Show Details)

Updated patch to just bail out if address spaces don't match.