This is an archive of the discontinued LLVM Phabricator instance.

[GISel] Add new combines for G_FMUL
AbandonedPublic

Authored by mkitzan on Sep 17 2020, 4:20 PM.

Details

Summary

Patch adds eight new GICombineRules for G_FMUL:

  • G_FMUL(x, 0.0) -> 0.0
  • G_FMUL(x, 1.0) -> x
  • G_FMUL(x, -1.0) -> G_FSUB(0.0, x)
  • G_FMUL(G_FNEG(x), G_FNEG(y)) -> G_FMUL(x, y)
  • G_FMUL(G_FMUL(x, c0), c1) -> G_FMUL(x, c0 * c1) (unsafe fp math only)
  • G_FMUL(G_FADD(x, x), c) -> G_FMUL(x, c * 2.0) (unsafe fp math only)
  • G_FMUL(x, G_SELECT(G_FCMP(x > 0.0), -1.0, 1.0) -> G_FNEG(G_FABS(x))
  • G_FMUL(x, G_SELECT(G_FCMP(x > 0.0), 1.0, -1.0)) -> G_FABS(x)

Patch additionally adds new combine tests for AArch64 target for these new rules, as well as updating AMDGPU GISel tests.

Diff Detail

Event Timeline

mkitzan created this revision.Sep 17 2020, 4:20 PM
mkitzan requested review of this revision.Sep 17 2020, 4:20 PM
arsenm added inline comments.Sep 17 2020, 4:22 PM
llvm/include/llvm/CodeGen/GlobalISel/CombinerHelper.h
358–359

This is incorrect since it replaces a canonicalizing operation with a non-canonicalizing operation

mkitzan added inline comments.Sep 17 2020, 4:24 PM
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
2206

Decided against using the new matchOperandIsFPZero here, because it would require matchOperandIsFPZero not to validate that the FConstant operand could replace the dst of its use (which would diverge from the semantics of matchOperandIsZero).

mkitzan added inline comments.Sep 17 2020, 4:36 PM
llvm/include/llvm/CodeGen/GlobalISel/CombinerHelper.h
358–359

Worth noting there is an identical DAGCombiner for this though it was committed 13yrs ago. Is this a GISel specific requirement (not replacing a canonicalizing op with a non-canonicalizing op)? Regardless, it can easily be turned into G_FMUL(x, -1.0) -> G_FSUB(0.0, x).

arsenm added inline comments.Sep 17 2020, 4:41 PM
llvm/include/llvm/CodeGen/GlobalISel/CombinerHelper.h
358–359

Until recently, fneg and fsub -0, x were considered identical and didn't try to get this right. The DAG is broken here

mkitzan added inline comments.Sep 17 2020, 4:47 PM
llvm/include/llvm/CodeGen/GlobalISel/CombinerHelper.h
358–359

I see. I was wondering about the discrepancy between the G_MUL version and this one, but chalked it up floating point stuff. I'll post a fixup changing it to fold into G_FSUB(0.0, x).

mkitzan updated this revision to Diff 292677.Sep 17 2020, 5:25 PM
mkitzan edited the summary of this revision. (Show Details)
  • Changed G_FMUL(x, -1.0) to fold into G_FSUB(0.0, x)
  • Fixed clang-tidy warnings
  • Changed some fp constants in comments from {n} to {n}.0
jurahul added inline comments.
llvm/include/llvm/Target/GlobalISel/Combine.td
299

Sorry to comment on a change that I know nothing about, but ts this somewhere checking if fast math or equivalent is enabled? Otherwise, X * 0.0 != 0.0 in floating point.

mkitzan added inline comments.Sep 18 2020, 1:17 PM
llvm/include/llvm/Target/GlobalISel/Combine.td
299

At the moment nothing is checking whether fast math of special flags are enabled, but perhaps it should.

arsenm added inline comments.Sep 18 2020, 1:38 PM
llvm/include/llvm/Target/GlobalISel/Combine.td
299

This probably shouldn't be generalized to "binop". I think this is just fadd and fsub, and both require nsz (although actually I think you don't need nsz for -0)

mkitzan added inline comments.Sep 18 2020, 3:10 PM
llvm/include/llvm/Target/GlobalISel/Combine.td
299

I choose to generalize it as a binop because the G_MUL version was implemented in this way (https://reviews.llvm.org/D80094).

arsenm added inline comments.Oct 7 2020, 7:29 AM
llvm/include/llvm/Target/GlobalISel/Combine.td
299

The integer and FP operations have little to with each other, so I don't think following how they work is necessarily the best example

arsenm added a comment.Nov 2 2020, 9:15 AM

reverse ping

mkitzan updated this revision to Diff 303959.Nov 9 2020, 12:35 PM

Renamed binop_right_to_fp_zero to fmul_by_zero

foad added a subscriber: foad.Nov 10 2020, 1:27 AM
foad added inline comments.
llvm/include/llvm/CodeGen/GlobalISel/CombinerHelper.h
358

I think this should be "G_FSUB(-0.0, x)".

llvm/include/llvm/Target/GlobalISel/Combine.td
491

This is wrong if x is Inf or NaN.

499

"-0.0".

I have a fixup ready with the G_FMUL(x, -1.0)->G_FSUB(-0.0, x) change. However, I just want to know in specific terms if there's anything else blocking approval.

foad added a comment.Jan 5 2021, 11:59 AM

Well (fmul x, 0.0) -> 0.0 is still wrong if x is Inf or NaN.

I feel like this would be a bit easier to get in if it was broken into 1 patch per combine?

Sure thing. I'll split this up into separate patches.

Herald added a project: Restricted Project. · View Herald TranscriptMay 19 2022, 11:03 AM
Herald added a subscriber: kosarev. · View Herald Transcript
mkitzan abandoned this revision.May 19 2022, 12:40 PM

Patch has been split up into sub-patches, closing this revision: