Page MenuHomePhabricator

[InstSimplify] fold fma/fmuladd with a NaN operand
ClosedPublic

Authored by spatel on Sep 18 2019, 10:40 AM.

Details

Summary

I'm not sure if/how adding these simplifications will impact D67351, but we should have the basic cases within InstSimplify because that potentially makes other passes like GVN better.

This is intended to be similar to the constant folding results from D67446 and earlier, but not all operands are constant in these tests, so the responsibility for folding is left to InstSimplify.

Diff Detail

Repository
rL LLVM

Event Timeline

spatel created this revision.Sep 18 2019, 10:40 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 18 2019, 10:40 AM
fhahn added a comment.Sep 18 2019, 2:16 PM

D67553 adds SimplifyFMAMul. We should probably also have them there. Also, could we make use of multiply by 0.0 here, with the required fast-math flags?

D67553 adds `SimplifyFMAMul`. We should probably also have them there. Also, could we make use of multiply by 0.0 here, with the required fast-math flags?

Yes, these patches should converge somehow. That's partly why I only put the NaN folding in this one, but if you think it's better to wait for the other patches to land, that's fine with me.
I think we'll end up modifying D67351 - if we can fold fma(a,0,b) --> b, then we want that to happen in InstSimplify directly rather than InstCombine because it's more efficient that way.

reames resigned from this revision.Sep 26 2019, 3:57 PM
spatel updated this revision to Diff 222291.Sep 28 2019, 6:38 AM

Patch updated:
The other fma-simplification-related patches have landed, but this limited patch is still independent of that.
I did change this patch to use the existing logic that we apply to normal FP ops, so now we fold 'undef' and 'NaN' operands consistently with the FP binops (fmul, fadd, fsub. fdiv, frem).

fhahn accepted this revision.Sep 30 2019, 2:41 PM
fhahn added a reviewer: mcberg2017.

Patch updated:
The other fma-simplification-related patches have landed, but this limited patch is still independent of that.

Right. I think we could also use simplifyFPOp in SimplifyFMAFMul.

Additionally,I think if the 'add' operand folds to a constant that does not require rounding, we could use SimplifyFMAFMul and try to simplify the whole fma to a constant.

I did change this patch to use the existing logic that we apply to normal FP ops, so now we fold 'undef' and 'NaN' operands consistently with the FP binops (fmul, fadd, fsub. fdiv, frem).

Great. The naming of simplifyFPOp is a bit unfortunate IMO, but it handles exactly the undef & nan operand cases . LGTM.

Please wait with committing a day or so, in case there are additional comments.

This revision is now accepted and ready to land.Sep 30 2019, 2:41 PM

Great. The naming of simplifyFPOp is a bit unfortunate IMO, but it handles exactly the undef & nan operand cases . LGTM.

I'm to blame for the name. I just recently changed it from simplifyFPBinOp(). :)
Definitely open to suggestions for a better name - prefer more exact?
simplifyFPWithUndefOrNaNOperand()

fhahn added a comment.Sep 30 2019, 2:58 PM

Great. The naming of simplifyFPOp is a bit unfortunate IMO, but it handles exactly the undef & nan operand cases . LGTM.

I'm to blame for the name. I just recently changed it from simplifyFPBinOp(). :)
Definitely open to suggestions for a better name - prefer more exact?
simplifyFPWithUndefOrNaNOperand()

Personally I think a more verbose name like the one suggested makes it a bit clearer (and the reader does not necessarily have to check the function implementation). But this is mostly a personal preference I think.

This revision was automatically updated to reflect the committed changes.