This is an archive of the discontinued LLVM Phabricator instance.

[LV] Don't sink scalar instructions that may read from memory.
ClosedPublic

Authored by fhahn on Mar 30 2023, 12:19 PM.

Details

Summary

The current sinking code doesn't prevent us from sinking a load past an
aliasing store. Skip sinking instructions that may read from memory to
avoid potential mis-compile.

See @minimal_bit_widths_with_aliasing_store for an example where 2 loads
are sunk past aliasing stores before this fix.

Diff Detail

Event Timeline

fhahn created this revision.Mar 30 2023, 12:19 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 30 2023, 12:19 PM
fhahn requested review of this revision.Mar 30 2023, 12:19 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 30 2023, 12:19 PM
Ayal accepted this revision.Apr 16 2023, 12:38 PM

Looks good to me, thanks for fixing! Worth mentioning that this fixes a bug?

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
4212–4213

nit: // ... or may read from memory.

Worth adding a TODO to allow sinking a load past non-store instructions? (Could potentially also sink obstructing stores as well(?)) Better during VPlan-to-VPlan sinking.

This revision is now accepted and ready to land.Apr 16 2023, 12:38 PM
This revision was landed with ongoing or failed builds.Apr 17 2023, 1:30 AM
This revision was automatically updated to reflect the committed changes.
fhahn marked an inline comment as done.Apr 17 2023, 1:33 AM

Looks good to me, thanks for fixing! Worth mentioning that this fixes a bug?

Thanks! There's no bug report I am aware of, but the commit message mentions that this avoids a mis-compile.

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
4212–4213

Thanks, added the comment + TODO.

Worth adding a TODO to allow sinking a load past non-store instructions? (Could potentially also sink obstructing stores as well(?)) Better during VPlan-to-VPlan sinking.

Agreed, possibly re-use the same analysis as for sinking memory instructions in the VPlan-based recurrence handling code.