[PATCH] [Reassociate] Keep NSW/NUW flags on binary ops whenever possible.

[Ahmed Bougacha]

How about simply never reassociating if the entire tree has consistent
nsw/nuw flags?

I'll have to run performance tests, but I think it might be worth it.

- Ahmed


On Mon, Nov 3, 2014 at 1:26 PM, Ahmed Bougacha <ahmed.bougacha at gmail.com> wrote:
> On Mon, Nov 3, 2014 at 1:19 PM, David Menendez <davemm at cs.rutgers.edu> wrote:
>> In general, re-associating add nsw changes the inputs where poison can happen. For example, (-2 + 1) + MAX_INT is okay, but -2 + (1+MAX_INT) is poison.
>>
>> I haven't had a chance to look through this patch in detail, so I don't know if there is any additional analysis going on.
>
> Darn it, I missed that kind of case! There isn't any analysis other
> than making sure the flags are the same, so I don't think the
> transform is actually valid anymore; I'll look into it.
>
> Thanks,
> - Ahmed
>
>>> On Nov 3, 2014, at 3:42 PM, Hal Finkel <hfinkel at anl.gov> wrote:
>>>
>>> David, Nuno,
>>>
>>> The Alive talk at the developers' meeting, as I recall, touched specifically on this issue. Could one of you please comment on the general validity of this?
>>>
>>> Thanks again,
>>> Hal
>>>
>>> ----- Original Message -----
>>>> From: "Ahmed Bougacha" <ahmed.bougacha at gmail.com>
>>>> To: "ahmed bougacha" <ahmed.bougacha at gmail.com>, hfinkel at anl.gov, chandlerc at gmail.com, grosbach at apple.com
>>>> Cc: llvm-commits at cs.uiuc.edu
>>>> Sent: Monday, November 3, 2014 12:44:06 PM
>>>> Subject: [PATCH] [Reassociate] Keep NSW/NUW flags on binary ops whenever possible.
>>>>
>>>> Hi hfinkel, chandlerc, grosbach,
>>>>
>>>> The previous - conservative - behavior was to always drop the
>>>> overflow flags
>>>> when reassociating scalar expressions.
>>>>
>>>> This kept address computing code done on int/i32 and sign-extended to
>>>> i64,
>>>> because the nsw/nuw flags are needed to promote.  In turn, this made
>>>> it
>>>> impossible to fold the address computation into the addressing mode.
>>>>
>>>> This (simple) patch tries to keep the nsw/nuw flags, but only when
>>>> they are
>>>> consistent in the complete expression tree. My understanding is, this
>>>> should be
>>>> valid, because the poison has to propagate to the root, no matter how
>>>> the
>>>> expression is reassociated.
>>>> My reading of the rest of the code tells me the expression tree only
>>>> consists of
>>>> the same operator, and all expressions have only one use.
>>>>
>>>> Now for measurements:
>>>>
>>>> Compile Time                                                                         Δ               Previous        Current σ
>>>> MultiSource/Benchmarks/ASC_Sequoia/IRSmk/IRSmk       49.81%  0.1321  0.1979
>>>>      0.0005
>>>> SingleSource/Benchmarks/Misc/himenobmtxpa                    7.40%   0.1839  0.1975
>>>>      0.0009
>>>>
>>>> Execution Time                                                                       Δ               Previous        Current σ
>>>> SingleSource/Benchmarks/Linpack/linpack-pc                   1.35%   2.3403  2.3719
>>>>      0.0033
>>>> MultiSource/Benchmarks/ASC_Sequoia/IRSmk/IRSmk       -8.58%  4.7386  4.3318
>>>>      0.0052
>>>> SingleSource/Benchmarks/Misc/himenobmtxpa                    -5.91%  1.6205  1.5248
>>>>      0.0178
>>>> MultiSource/Benchmarks/tramp3d-v4/tramp3d-v4         -2.39%  0.4302  0.4199
>>>>      0.0011
>>>>
>>>> I'm not exactly sure why linpack is slower; I'm (lightly)
>>>> investigating, but can
>>>> look into it more seriously if people think it's a big deal?
>>>>
>>>> Thanks,
>>>> -Ahmed
>>>>
>>>> http://reviews.llvm.org/D6097
>>>>
>>>> Files:
>>>>  lib/Transforms/Scalar/Reassociate.cpp
>>>>  test/Transforms/Reassociate/no-op.ll
>>>>  test/Transforms/Reassociate/nsw-nuw.ll
>>>>
>>>
>>> --
>>> Hal Finkel
>>> Assistant Computational Scientist
>>> Leadership Computing Facility
>>> Argonne National Laboratory
>>