Page MenuHomePhabricator

Re-enable "[SCEV] Do not fold dominated SCEVUnknown into AddRecExpr start"
ClosedPublic

Authored by mkazantsev on May 25 2017, 4:50 AM.

Details

Summary

The patch rL303730 was reverted because test lsr-expand-quadratic.ll failed on many non-X86 configs with this patch.
The reason of this is that the patch makes a correctless fix that changes optimizer's behavior for this test. Now, following
the right execution path, LSR tries to make a transformation relying on IV Users analysis. This analysis is target-dependent
due to this code:

// LSR is not APInt clean, do not touch integers bigger than 64-bits.
// Also avoid creating IVs of non-native types. For example, we don't want a
// 64-bit IV in 32-bit code just because the loop has one 64-bit cast.
uint64_t Width = SE->getTypeSizeInBits(I->getType());
if (Width > 64 || !DL.isLegalInteger(Width))
  return false;

To make a proper transformation in this test case, the type i32 needs to be legal for
the specified data layout. When the test runs on some non-X86 configuration
(e.g. pure ARM 64), opt gets confused by the specified target and does not use it,
rejecting the specified data layout as well. Instead, it uses some default layout that
does not treat i32 as a legal type (currently the layout that is used when it is not
specified does not have legal types at all). As result, the transformation we expect to
happen does not happen for this test.

This re-enabling patch does not have any source code changes compared to the
original patch rL303730. The only difference is that the failing test is moved to
X86 directory and now has requirement of running on x86 only to comply with
the specified target triple and data layout.

Diff Detail

Event Timeline

mkazantsev created this revision.May 25 2017, 4:50 AM
sanjoy edited edge metadata.May 25 2017, 5:01 PM

Hi Maxim,

I think there is one bit still unanswered -- in the case where we lack a proper TargetMachine (i.e. the case where we're running an ARM-only build on the lsr-expand-quadratic.ll test) we went from passing without your change to failing with your change. What changed? Was LSR relying on incorrect SCEV behavior (this won't surprise me)?

lib/Analysis/ScalarEvolution.cpp
2209

In a later change, maybe you can change this code to just do:

if (auto *SU = dyn_cast<SCEVUnknown>(S))
  return checkUnknown(...);
return true;

(there should be no need to return false on constants -- they have no operands and thus nothing to follow)

test/Transforms/LoopStrengthReduce/X86/lsr-expand-quadratic.ll
1

Not sure if the REQUIRES: x86 bit is needed -- this directory already has a lit.local.cfg that should have the same restriction.

Sanjoy, what happens is nearly following: without the change, LSR was making an overconfident simplification basing on a wrong SCEV. Apparently it did not need the IV analysis to do this. With the change, it chose a different way to simplify (that wasn't so confident), and this way required the IV analysis.

mkazantsev added inline comments.May 25 2017, 8:10 PM
lib/Analysis/ScalarEvolution.cpp
2209

I keep this swich intentionally. If a new type o SCEV ever appears, we will have a warning here. Addressing this warning should motivate the author of new SCEV to pay attention to this situation and process this new node properly.

sanjoy accepted this revision.May 25 2017, 9:49 PM

Sanjoy, what happens is nearly following: without the change, LSR was making an overconfident simplification basing on a wrong SCEV. Apparently it did not need the IV analysis to do this. With the change, it chose a different way to simplify (that wasn't so confident), and this way required the IV analysis.

Please mention this in the commit message.

This revision is now accepted and ready to land.May 25 2017, 9:49 PM