Page MenuHomePhabricator

[DAGCombiner] Don't fold FCOPYSIGN vector sign operand casts
ClosedPublic

Authored by luismarques on Feb 4 2021, 7:23 AM.

Details

Summary

Avoid doing the following combine for vector types:

copysign(x, fp_extend(y)) -> copysign(x, y)
copysign(x, fp_round(y)) -> copysign(x, y)

That combine seemed to impede the selection of vector instruction and cause a mess in some circumstances.

This patch was extracted from D70426, which I am abandoning. The patch also refactors the code, for clarity.

Diff Detail

Event Timeline

luismarques created this revision.Feb 4 2021, 7:23 AM
luismarques requested review of this revision.Feb 4 2021, 7:23 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 4 2021, 7:23 AM
luismarques edited the summary of this revision. (Show Details)Feb 4 2021, 7:36 AM

FWIW, something this would likely help with D96028, as RVV doesn't benefit from this fpextend/fptrunc folding. We have to undo it in the custom-lowering and use a custom-node to prevent infinite loops. I was wondering if the combiner should ask the target whether it's beneficial to fold the ext/trunc or not?

FWIW, something this would likely help with D96028, as RVV doesn't benefit from this fpextend/fptrunc folding. We have to undo it in the custom-lowering and use a custom-node to prevent infinite loops. I was wondering if the combiner should ask the target whether it's beneficial to fold the ext/trunc or not?

I did propose that in the past: https://reviews.llvm.org/D66725
I may have misunderstood the review feedback I got back then (spread over a variety of patches and personal email), but that feedback led to me abandoning that patch.
I think part of the problem, at the time, was that my patches stressed too much the issues around the legality of the operation/operands, and not enough about the combine being desirable.
When I re-analyzed this issue, before submitting this patch, it did seem to me that a hook would still be beneficial, even if many of the original problems had since been addressed by other means.

This revision is now accepted and ready to land.Feb 8 2021, 10:49 PM
frasercrmck accepted this revision.Feb 9 2021, 2:08 AM

I did propose that in the past: https://reviews.llvm.org/D66725
I may have misunderstood the review feedback I got back then (spread over a variety of patches and personal email), but that feedback led to me abandoning that patch.
I think part of the problem, at the time, was that my patches stressed too much the issues around the legality of the operation/operands, and not enough about the combine being desirable.
When I re-analyzed this issue, before submitting this patch, it did seem to me that a hook would still be beneficial, even if many of the original problems had since been addressed by other means.

I see. I think the problem with implementing a hook is that it appears that no in-tree target benefits from this fcopysign optimization on vector types. So while I think it's the right idea in principle it's essentially the same as what you've got here as everyone would return false. I think this can go in as-is and a hook can come along later if a target wants it.

This revision was landed with ongoing or failed builds.Feb 10 2021, 6:26 AM
This revision was automatically updated to reflect the committed changes.