Page MenuHomePhabricator

[LoadCombine] Fix combining of loads which span an aliasing store.
ClosedPublic

Authored by Bigcheese on Jan 19 2017, 2:37 PM.

Details

Summary

Fixes PR31517

I'm not 100% sure about this fix. The problem is we don't check if the last load aliases anything, so this now checks all instructions to see if they alias and adds stores to the alias set. However I'm not sure if memcpy/memset or any other ways of writing need to be handled specially here.

Diff Detail

Repository
rL LLVM

Event Timeline

Bigcheese created this revision.Jan 19 2017, 2:37 PM

See also https://reviews.llvm.org/D21496.

Special-casing store instructions doesn't make any sense; any instruction which writes to memory has the same properties here.

davide edited edge metadata.Jan 20 2017, 11:51 AM

Yes, I think you should revamp D21496.

davide requested changes to this revision.Jan 20 2017, 11:53 AM
This revision now requires changes to proceed.Jan 20 2017, 11:53 AM
Bigcheese updated this revision to Diff 85216.Jan 20 2017, 4:48 PM
Bigcheese edited edge metadata.

Handle any instruction which may write to memory.

I don't know why but this patch seems to also make https://llvm.org/bugs/show_bug.cgi?id=31493 stop crashing.

I don't know why but this patch seems to also make https://llvm.org/bugs/show_bug.cgi?id=31493 stop crashing.

Can you please also try with the code in D21496 ? IIRC it was ready modulo tests.

I don't know why but this patch seems to also make https://llvm.org/bugs/show_bug.cgi?id=31493 stop crashing.

Can you please also try with the code in D21496 ? IIRC it was ready modulo tests.

Ok, PR31493 also stops crashing if applying only D21496.

This is definitely a stop-gap solution to the problem, and although we seem to have a more general patch already available, you don't have time/don't want to work on it.
As this fixes a miscompile and a crash reported upstream, I'm inclined to see this patch or a modified form of it upstream. River can land his version once he starts working on LLVM again (which should, presumably, happen soon). The reason why I'd like to see pushing forward is that we can backport the "fix" to the 4.0 branch. Eli, WDYT?

Why do you want to backport this to 4.0? LoadCombine isn't part of the default pass pipeline.

test/Transforms/LoadCombine/load-combine-aa.ll
48 ↗(On Diff #85216)

Check for the second load after the store?

Why do you want to backport this to 4.0? LoadCombine isn't part of the default pass pipeline.

While the pass is not part of the default pipeline, some people use it (and apparently breaks).
That said, on a second thought, I decide to withdraw my request to backport this as it doesn't seem there's a reason strong enough to do that.
Also, the crash has been there since forever as far as I can tell, in fact I can reproduce it pre 3.9.

Ka-Ka edited edge metadata.Feb 8 2017, 8:00 AM

Any news about this issue?

test/Transforms/LoadCombine/load-combine-aa.ll
48 ↗(On Diff #85216)

The testcase run first the load-combine pass and then instcombine pass. The instcombine pass will remove the second load after the store. If you think it is important to check for the second load then the testcase need to be splitted from the other tests and the instcombine pass need to be turned off.

efriedma added inline comments.Feb 8 2017, 9:56 AM
test/Transforms/LoadCombine/load-combine-aa.ll
48 ↗(On Diff #85216)

Probably the whole file should be fixed not to run instcombine.

davide added a comment.Feb 9 2017, 1:48 PM

Are you still working on this?

Cheers,

Davide

This revision was automatically updated to reflect the committed changes.