This is an archive of the discontinued LLVM Phabricator instance.

[LSR] Check SCEV on isZero() after extend. PR40514
ClosedPublic

Authored by mkazantsev on Jan 31 2019, 10:17 PM.

Details

Summary

When LSR first adds SCEVs to BaseRegs, it only does it if isZero() has returned false.
In the end, in invocation of InsertFormula, it asserts that all values there are still not
zero constants. However between these two points, it makes some transformations,
in particular extends them to wider type.

SCEV does not give us guarantee that if S is not a constant zero, then sext(S) is
also not a constant zero. It might have missed some optimizing transforms when it
was calculating S and then made them when it took sext. For example, it may
happen if previously optimizing transforms were limited by depth or somehow else.

This patch adds a bailout when we may end up with a zero SCEV after extension.

Diff Detail

Event Timeline

mkazantsev created this revision.Jan 31 2019, 10:17 PM

Hi Max,

Why don't be just return false from InsertFormula if we find zero?

Let me check if it works. I am not familiar well enough with LSR's logic, to me it seemed like we tried really hard to *not* allow zeros here (but still end up with them).

mkazantsev retitled this revision from [LSR] Remove overly confident asserts on isZero(). PR40514 to [LSR] Check SCEV on isZero() after extend. PR40514.
mkazantsev edited the summary of this revision. (Show Details)

You were right, we can keep asserts and make a bailout at the point where we take extensions.

samparker accepted this revision.Feb 4 2019, 3:03 AM

Cheers, LGTM.

lib/Transforms/Scalar/LoopStrengthReduce.cpp
3971

typo: while

This revision is now accepted and ready to land.Feb 4 2019, 3:03 AM
mkazantsev marked an inline comment as done.Feb 4 2019, 8:23 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 4 2019, 8:30 PM