Page MenuHomePhabricator

[SCEV] Clear poison flags during expansion of SCEV
Needs ReviewPublic

Authored by skatkov on Jan 19 2018, 3:26 AM.



This is an alternate solution to rL322058 which causes performance
degradation according to report from @eastig.

Generally when we expand a SCEV and re-use the instruction we
check whether SCEV has lost a poison flags but instruction has.
In this case we just clear poison flags from instruction.

This is pretty similar what GVN will do if we we literally expand the SCEV
without poison flags.

Potentially it might have negative performance impact due to we clear
nuw/nsw from instruction and possible loose some performance

Diff Detail

Event Timeline

skatkov created this revision.Jan 19 2018, 3:26 AM
skatkov updated this revision to Diff 131029.Jan 23 2018, 3:30 AM

Added regression test. Taken from @eastig.

sanjoy added inline comments.



This is bothering me -- I agree that this mitigation is better than nothing, but what you've found shows that this FindValueInExprValueMap business is basically incorrect, except in perhaps very limited cases.

I'm tempted to rip all of this reuse logic out of SCEV/SCEVExpander. Back when this was added in we talked about improving our CSE as an alternative to caching expressions like this; I think this bug is a sign that we should, in fact, seriously consider what FindValueInExprValueMap does as a CSE optimization.

+CC @wmi @hfinkel for discussion.

skatkov added inline comments.Jan 24 2018, 12:14 AM

Just a side note. This functionality is used in backend as well (in loop reduce pass). If I understand correctly your proposal is to eliminate caching and just always literally expand the SCEV and CSE later will eliminate all redundant instructions. But in this case we will need to add a CSE pass after loop reduce as well.

Ok, I keep looking into your discussion.

sanjoy added inline comments.Jan 24 2018, 9:24 AM

But in this case we will need to add a CSE pass after loop reduce as well.

Or we can instead do some basic CSE during SCEV expansion (we already do a bit of this).

In any case, correctness trumps everything. :)

Hi @sanjoy @wmi @hfinkel, is there any discussion ongoing about this patch? There is more than month without progress, should we maybe start a discussion here?

I'm personaly also inclined to think that this caching logic is wrong and we should get rid of it, but will clearly have consequences from compile time point, so it's definitely worth discussing.