Do not drop fast-math flags in SimplifyBinOp

[Chandler Carruth]

On Thu, Feb 5, 2015 at 5:36 PM, Michael Zolotukhin <mzolotukhin at apple.com>
wrote:

> Hi Chandler,
>
> We now have a new user of SimplifyBinOp in LoopUnroll, I’ve updated the
> patch accordingly:
>
>
>
> Does it look good?
>

One issue:

+static Value *SimplifyFPBinOp(unsigned Opcode, Value *LHS, Value *RHS,
+                              const Query &Q, unsigned MaxRecurse,
+                              const FastMathFlags &FMF) {
+  switch (Opcode) {
+  case Instruction::FAdd:
+    return SimplifyFAddInst(LHS, RHS, FMF, Q, MaxRecurse);

We shouldn't have different argument ordering between the internal
SimplifyFPBinOp and SimplifyFAddInst. Pass FMF after RHS in SimplifyFPBinOp?

LGTM with that change.


>
> Thanks,
> Michael
>
> On Jan 30, 2015, at 12:10 PM, Michael Zolotukhin <mzolotukhin at apple.com>
> wrote:
>
> Hi Chandler,
>
> Thanks for the feedback, updated patch is attached.
>
> I added SimplifyFPBinOp routine, but decided to keep SimplifyBinOp
> untouched. I.e. if we use SimplifyBinOp for FP operator, we just use
> default fast math flags - that’s what we have currently. It doesn’t lead to
> incorrect code, but it’s conservative, and it’s better to use
> SimplifyFPBinOp with explicitly specified flags. I’m not sure if that’s
> what you meant though - are you fine with this approach?
>
> As for clang format - I did use it last time, and used this time as well
> (I believe the indentation was strange, because clang-format doesn’t always
> try to fold several short lines that could fit to 80 characters if placed
> in one line).
>
>
> <inst-simplify-fast-math-3.patch>
>
> Thanks,
> Michael
>
>
> On Jan 29, 2015, at 6:13 PM, Chandler Carruth <chandlerc at google.com>
> wrote:
>
> Yea, I think this is the right direction.
>
> Let's clean up how the API is structured though:
>
> 1) pass FastMathFlags by value, and default construct the obvious value.
>
> 2) what about making a SimplifyFPBinOp which *requires* a flags parameter
> in addition to the opcode? Then SimplifyBinOp could delegate to this with a
> default constructed flags for the FP cases. In the caller code, I feel like
> this would be a touch cleaner / less prone to minor errors. Because we
> already have to test whether the operation is FP and could have flags, we
> might as well call a specialized routine.
>
> As a nit-pick, it'd be nice to use clang-format on the caller, I think it
> will do a nice job with these long argument lists. (Unless you did, and
> clang-format is just choosing this... somewhat surprising layout?)
>
> On Thu, Jan 29, 2015 at 6:02 PM, Michael Zolotukhin <mzolotukhin at apple.com
> > wrote:
>
>>
>> On Jan 29, 2015, at 6:01 PM, Michael Zolotukhin <mzolotukhin at apple.com>
>> wrote:
>>
>>
>> On Jan 29, 2015, at 4:21 PM, Chandler Carruth <chandlerc at google.com>
>> wrote:
>>
>> Do we use the context instruction to extract fast math flags elsewhere?
>>
>> I wouldn't expect that to be reliably at all. Nothing *requires* the
>> context instruction to be the one governing the semantics.
>>
>> I think we need to pass the fast math flags directly into SimplifyBinOp
>> for this to work.
>>
>>
>> I tried to avoid adding a new argument, but you are right: using the
>> context instruction isn’t appropriate for this.
>>
>> Updated patch is attached, is it better?
>>
>> Oops, I forgot the patch. Here it is:
>>
>>
>>
>> Michael
>>
>>
>>
>>
>> Michael
>>
>>
>> On Thu, Jan 29, 2015 at 4:12 PM, Michael Zolotukhin <
>> mzolotukhin at apple.com> wrote:
>>
>>> Hi,
>>>
>>> Currently InlineCost pass fails to use fast-math flags when analyzing
>>> profitability of inlining. Here is a small example to demonstrate that:
>>>
>>> float foo(float *a, float b) {
>>>   float r;
>>>   if (a[1] * b)
>>>     r = a[1]+a[2]/a[3]-a[4]*a[5]+a[6]-a[7]+a[8]*a[9]+
>>>             a[11]+a[12]/a[13]-a[14]*a[15]+a[16]-a[17]+a[18]*a[19]+
>>>             a[21]+a[22]/a[23]-a[24]*a[25]+a[26]-a[27]+a[28]*a[29]+
>>>             a[31]+a[32]/a[33]-a[34]*a[35]+a[36]-a[37]+a[38]*a[39]+
>>>             a[41]+a[42]/a[43]-a[44]*a[45]+a[46]-a[47]+a[48]*a[49]+
>>>             a[51]+a[52]/a[53]-a[54]*a[55]+a[56]-a[57]+a[58]*a[59]+
>>>             a[61]+a[62]/a[63]-a[64]*a[65]+a[66]-a[67]+a[68]*a[69]+
>>>             a[0]*a[0]+b;
>>>   else
>>>     r = 1;
>>>   return r;
>>> }
>>> float boo(float *a) {
>>>   return foo(a, 0.0);
>>> }
>>>
>>> Here inliner can’t figure out that a[1]*b can be folded to 0 if the call
>>> is inlined, and decides not to inline.
>>>
>>> That happens because inliner drops fast-math flags in SimplifyBinOp. The
>>> attached patch fixes it by looking into the instruction math flags. Does it
>>> look good?
>>>
>>> Without patch:
>>> > clang inline.c -O3 -S -Rpass=inline -ffast-math
>>> // Empty output
>>>
>>> With patch:
>>> > clang inline.c -O3 -S -Rpass=inline -ffast-math
>>> inline.c:17:10: remark: foo inlined into boo [-Rpass=inline]
>>>   return foo(a, 0.0);
>>>          ^
>>>
>>>
>>>
>>>
>>> Thanks,
>>> Michael
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>>
>>>
>>
>>
>>
>>
>
> _______________________________________________
> 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/20150206/ac3593ec/attachment.html>