Do not drop fast-math flags in SimplifyBinOp
Michael Zolotukhin
mzolotukhin at apple.com
Fri Feb 6 12:24:40 PST 2015
> On Feb 6, 2015, at 11:07 AM, Chandler Carruth <chandlerc at google.com> wrote:
>
>
>
> On Thu, Feb 5, 2015 at 5:36 PM, Michael Zolotukhin <mzolotukhin at apple.com <mailto: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, Chandler! Fixed and committed in r228432. I’ll follow up with a test case soon.
Michael
>
>
> Thanks,
> Michael
>> On Jan 30, 2015, at 12:10 PM, Michael Zolotukhin <mzolotukhin at apple.com <mailto: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 <mailto: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 <mailto:mzolotukhin at apple.com>> wrote:
>>>
>>>> On Jan 29, 2015, at 6:01 PM, Michael Zolotukhin <mzolotukhin at apple.com <mailto:mzolotukhin at apple.com>> wrote:
>>>>
>>>>
>>>>> On Jan 29, 2015, at 4:21 PM, Chandler Carruth <chandlerc at google.com <mailto: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 <mailto: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 <mailto:llvm-commits at cs.uiuc.edu>
>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits <http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits>
>>>>>
>>>>>
>>>>
>>>
>>>
>>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu <mailto:llvm-commits at cs.uiuc.edu>
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits <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/5c6c4e5d/attachment.html>
More information about the llvm-commits
mailing list