Page MenuHomePhabricator

[GISel] Add new combines for G_FMUL
Needs ReviewPublic

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



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

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

mkitzan added inline comments.Sep 17 2020, 4:24 PM

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

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

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

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.

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

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

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

I choose to generalize it as a binop because the G_MUL version was implemented in this way (

arsenm added inline comments.Oct 7 2020, 7:29 AM

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.Mon, Nov 2, 9:15 AM

reverse ping

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

Renamed binop_right_to_fp_zero to fmul_by_zero

foad added a subscriber: foad.Tue, Nov 10, 1:27 AM
foad added inline comments.

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


This is wrong if x is Inf or NaN.