This is an archive of the discontinued LLVM Phabricator instance.

[SROA] pr37267: fix assertion failure while integer widening
ClosedPublic

Authored by inouehrs on May 11 2018, 5:36 AM.

Details

Summary

The current integer widening does not support rewriting partial split spice in rewriteIntegerStore (and rewriteIntegerLoad).
This patch adds explicit check for this in isIntegerWideningViableForSlice.
Before r322533, splitting is allowed only for the whole-alloca slice and hence the above case is implicitly rejected by another check if (DL.getTypeStoreSize(ValueTy) > Size) because whole-alloca slice is larger than the partition.

By adding this case, there was no visible difference in the number of widening happened during the bootstrap test.

Diff Detail

Event Timeline

inouehrs created this revision.May 11 2018, 5:36 AM

This indeed solves the crash I saw. Thanks!

efriedma added inline comments.May 14 2018, 3:34 PM
lib/Transforms/Scalar/SROA.cpp
1994

Why does this check belong here, as opposed to the beginning of the function or somewhere in isIntegerWideningViable()?

test/Transforms/SROA/pr37267.ll
6

Please add FileCheck lines to show how we split the alloca. A test like this doesn't really make it clear what you're expecting to happen instead of crashing.

inouehrs updated this revision to Diff 146754.May 15 2018, 12:10 AM
  • modified test case
inouehrs added inline comments.May 15 2018, 12:27 AM
lib/Transforms/Scalar/SROA.cpp
1994

visitMemTransferInst seems to be able to handle split tails of memcpy (e.g. when replacing a store to slice 1 in the test case with memcpy). So I do not want to reduce the opportunities by checking this at the beginning of isIntegerWideningViableForSlice.

test/Transforms/SROA/pr37267.ll
6

I modified test case with FileCheck lines. Also I add comments to make the behavior clearer.

Your testcase doesn't actually crash on trunk, unlike the testcase from the bug.

lib/Transforms/Scalar/SROA.cpp
1994

So the limitation isn't that the transform is impossible, just that rewriteIntegerStore doesn't know how to insert the right shift? If that's correct, could you state that in the comment a little more explicitly? If not, could you explain a bit more why this isn't possible?

The testcase from the bug specifically hits the store case; can you construct a testcase which hits the load case?

inouehrs updated this revision to Diff 147022.May 16 2018, 2:28 AM
  • added a test case
inouehrs updated this revision to Diff 147040.May 16 2018, 3:48 AM
  • comments updated

Your testcase doesn't actually crash on trunk, unlike the testcase from the bug.

I fixed the test case. Thank you for pointing this out.

lib/Transforms/Scalar/SROA.cpp
1994

I added the second test case which causes an assertion failure while rewriting a load.

efriedma accepted this revision.May 16 2018, 11:31 AM

LGTM with one minor comment

lib/Transforms/Scalar/SROA.cpp
1953

Instead of doing this implicit signed-unsigned conversion, could you explicitly check "S.beginOffset() < AllocBeginOffset"? It's confusing to have to worry about overflow. (Granted, I don't think the overflowing case is likely to show up in practice, but there isn't anything preventing it.)

This revision is now accepted and ready to land.May 16 2018, 11:31 AM
This revision was automatically updated to reflect the committed changes.