[PATCH] PR17473: Disallow LSR to perform non-invertible normalization

[Eric Christopher]

On Tue, Mar 11, 2014 at 6:28 PM, Andrew Trick <atrick at apple.com> wrote:

>
> On Mar 11, 2014, at 4:31 PM, Michael Zolotukhin <mzolotukhin at apple.com>
> wrote:
>
> Hi Andy,
>
> I’ve run specs+test-suite with the same compiler (which aborts when LSR
> wants to discard a user), and got the following list of fails:
>
> New Failures - Compile Time ▾External/SPEC/CINT2000/254_gap/254_gap
> External/SPEC/CINT2000/255_vortex/255_vortex
> External/SPEC/CINT2006/400_perlbench/400_perlbench
> External/SPEC/CINT95/147_vortex/147_vortex
> MultiSource/Applications/ClamAV/clamscan
> MultiSource/Benchmarks/7zip/7zip-benchmark
> MultiSource/Benchmarks/mediabench/mpeg2/mpeg2dec/mpeg2decode
>
> Then I tried a possible fix, that we discussed yesterday: I added
> normalization of the stride before subtracting/adding it during
> NormalizeAutodetect/Denormalize. However, that fixed only one fail, the one
> from ‘make check’, described in the previous mail. The fails in specs and
> tests from test-suite persisted.
>
> I again looked at a couple of expressions, which we are failing to
> denormalize to the original version, and they were like this one:
>    ORIGINAL ISE: {(trunc i64 (1 + %i.0.ph) to i32),+,1}<%while.cond>
>    NORMALIZED ISE: {(trunc i64      %i.0.ph  to i32),+,1}<%while.cond>
>    DENORMALIZED ISE: {(1 + (trunc i64 %i.0.ph to i32)),+,1}<%while.cond>
>
> I.e. we didn’t move constants to trunc. I suppose this behavior is correct.
>
> Thus, I think we could commit the original patch, and then I’ll send a
> patch, which adds step normalization. What do you think?
>
>
> I was aware that SCEV is unable to canonicalize truncs. That's what made
> me nervous about this patch, which assumes canonical expressions. However,
> I don’t think this problem is worth changing your fix. I agree:
>
> (1) checkin this patch to get the bug fix in
>
> (2) follow up with the stride normalization fix
>
> and I would add
>
> (3) File a radar to investigate normalizing truncs, explaining how to
> easily reproduce the problem. We could probably fix this issue, but I’m
> nervous about it affecting SCEV consumers that already assume certain
> behavior. It will need even more in depth performance analysis.
>
>
Ahem. "File an external bug report and clone that into a radar." :)

-eric


> -Andy
>
> Thanks,
> Michael
>
> On Mar 10, 2014, at 5:55 PM, Andrew Trick <atrick at apple.com> wrote:
>
>
> On Mar 10, 2014, at 5:12 PM, Michael Zolotukhin <mzolotukhin at apple.com>
> wrote:
>
> I’ve run 'make check-all’ with llvm_unreachable in the branch where we
> discard a user, and actually got some interesting result. There were two
> failing tests, one of which is obviously the just added test.
>
> The second one is CodeGen/X86/lsr-normalization.ll, and here is what I got
> there:
>
>    ORIGINAL ISE: {(100 /u {1,+,1}<%bb16>),+,(100 /u
> {1,+,1}<%bb16>)}<%bb25>
>    NORMALIZED ISE: {((-1 * (100 /u {1,+,1}<%bb16>)) + (100 /u
> {0,+,1}<%bb16>)),+,(100 /u {0,+,1}<%bb16>)}<%bb25>
>    DENORMALIZED BACK ISE: {((2 * (100 /u {1,+,1}<%bb16>)) + (-1 * (100 /u
> {2,+,1}<%bb16>))),+,(100 /u {1,+,1}<%bb16>)}<%bb25>
>
> This could give an idea of what opportunities we could lose due to this
> change. Maybe we need to act so conservatively only in danger of overflow,
> i.e. when the original expression has sign-extensions/wrap-flags?
>
>
> Your current fix is conservatively correct. However, it also caught a more
> pervasive bug (the original bug only had to do with overflow at the
> induction variable’s boundary). As a follow-up, I agree with your
> suggestion to fix normalization so that it recursively normalizes the
> stride before subtracting it from the initial value. Your
> normalize-denormalize round trip check should also be sufficient to test
> that fix (you should only be eliminating cases in which the check fails,
> not adding any new ones).
>
> -Andy
>
> Thanks,
> Michael
>
> On Mar 10, 2014, at 5:01 PM, Andrew Trick <atrick at apple.com> wrote:
>
>
> On Mar 10, 2014, at 5:00 PM, Eric Christopher <echristo at gmail.com> wrote:
>
> As we discussed, once you have finished running the test-suite with a check
> to find out if any important cases are no longer normalized as expected,
> then I think you can commit. I know you've already verified performance, so
> this is just an extra sanity check.
>
>
> Any interesting results?
>
>
> No performance changes, just fixing PR17473.
>
> -Andy
>
>
> -eric
>
> Thanks.
>
> -Andy
>
>
> Thanks,
> Michael
>
> Thanks,
> Arnold
>
> On Mar 10, 2014, at 2:56 PM, Michael Zolotukhin <mzolotukhin at apple.com>
> wrote:
>
> Hi,
>
> This is a fix for PR17473. The issue is that LSR performs normalization of
> detected IV users and sometimes wants to denormalize it back hoping to get
> the original expression. But normalization isn't always invertible, and the
> new expression might be not equivalent to the original one. In the test
> case, we were losing sign-extension in such transformation. The patch
> simply
> adds a check, if the transformation is invertible.
>
> <pr17473.patch>
>
> Ok for trunk?
>
> Thanks,
> Michael
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
>
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140311/a4bbc005/attachment.html>