This is an archive of the discontinued LLVM Phabricator instance.

[LV] Don't assume isScalarAfterVectorization if one of the uses needs widening.
ClosedPublic

Authored by sdesmalen on Jul 16 2021, 10:16 AM.

Details

Summary

This fixes an issue that was found in D105199, where a GEP instruction
is used both as the address of a store, as well as the value of a store.
For the former, the value is scalar after vectorization, but the latter
(as value) requires widening.

Other code in that function seems to prevent similar cases from happening,
but it seems this case was missed.

Diff Detail

Event Timeline

sdesmalen created this revision.Jul 16 2021, 10:16 AM
sdesmalen requested review of this revision.Jul 16 2021, 10:16 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 16 2021, 10:16 AM
fhahn added inline comments.Jul 19 2021, 2:00 AM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
5205–5206

I think the actual problem is here.

Update should be inserted into ScalarPtrs. That way it will be only inserted into the work list if it is not identified as possibly non scalar. The comment above would also need updating accordingly. In this specific case, we should not treat pointer induction updates as scalar due to non-scalar uses.

sdesmalen updated this revision to Diff 360119.Jul 20 2021, 7:17 AM

Instead of removing a scalar pointer from the worklist, prevent it from being added.

sdesmalen updated this revision to Diff 360125.Jul 20 2021, 7:33 AM

Please ignore the previous revision :)

Changed patch to use ScalarPtrs and updated comment.

david-arm accepted this revision.Jul 23 2021, 5:55 AM

LGTM! Could you address the nit before merging? Thanks. :)

llvm/test/Transforms/LoopVectorize/AArch64/sve-widen-gep.ll
78

nit: I think you can delete !4 -> !7 now.

This revision is now accepted and ready to land.Jul 23 2021, 5:55 AM
fhahn added inline comments.Jul 23 2021, 5:57 AM
llvm/test/Transforms/LoopVectorize/AArch64/sve-widen-gep.ll
4

Do we still need this test? The issue is not SVE/AArch64 specific and it should already be covered by the general Test in Pointer-induction.ll.

sdesmalen added inline comments.Jul 23 2021, 6:09 AM
llvm/test/Transforms/LoopVectorize/AArch64/sve-widen-gep.ll
4

I'm a bit cautious about someone just regenerating the pointer-induction.ll test and reviewers not realising this is a big functional regression, because for scalable vectors it actually causes something to break. This test would guard that.

This revision was landed with ongoing or failed builds.Jul 26 2021, 8:02 AM
This revision was automatically updated to reflect the committed changes.
fhahn added inline comments.Jul 26 2021, 8:24 AM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
5228

This change should not be needed I think; the changes above should be sufficient.

llvm/test/Transforms/LoopVectorize/AArch64/sve-widen-gep.ll
4

ok fair enough.

57

I think would be good to either assign more meaningful names to make the test a bit easier to read or use the same version as in llvm/test/Transforms/LoopVectorize/pointer-induction.ll.