Page MenuHomePhabricator

[DAGCombiner][RISCV] Avoid FCOPYSIGN folding of legalizing operand casts
AbandonedPublic

Authored by luismarques on Nov 19 2019, 2:52 AM.

Details

Summary

This patch is an alternative to D66725. It provides some minimal changes that allow solving the RISC-V fcopysign issues without being blocked on larger refactoring/restructuring changes such as those of D66725 or D70064.

It gates the DAG combine folding of fp_extend/fp_round "casts" of an fcopysign sign operand when doing so would produce an operand value with an illegal type. (The folding is still done when the original sign value already had an illegal type, as in that case we expect the fcopysign to be expanded and we avoid libcalls for the value extension/rounding).

For clarity, the patch includes the RISC-V tests that motivated the DAGCombiner changes, although those can be committed separately.

Diff Detail

Event Timeline

luismarques created this revision.Nov 19 2019, 2:52 AM
luismarques edited the summary of this revision. (Show Details)Nov 19 2019, 4:04 AM
brooks added a subscriber: brooks.Nov 19 2019, 10:27 AM
lenary added inline comments.Nov 20 2019, 3:14 AM
llvm/lib/Target/RISCV/RISCVInstrInfoD.td
234

Please can you add a comment to note that these patterns will eventually not be needed, once we improve fcopysign handling?

luismarques marked an inline comment as done.Nov 20 2019, 3:32 AM
luismarques added inline comments.
llvm/lib/Target/RISCV/RISCVInstrInfoD.td
234

Although that was my original plan, there doesn't seem to be review consensus yet that such changes will be coming and accepted. (If only this patch was committed we would be left at a reasonable place, even if not perfect.)

asb added inline comments.Nov 21 2019, 3:46 AM
llvm/lib/Target/RISCV/RISCVInstrInfoD.td
234

I take your point about not wanting to describe future plans where there might not be consensus. I'd put a brief sentence like "These patterns can be removed if..."

lenary resigned from this revision.Jan 14 2021, 11:08 AM
rkruppe removed a subscriber: rkruppe.Jan 15 2021, 12:55 AM
david-arm accepted this revision.Jan 26 2021, 12:31 AM
david-arm added a subscriber: david-arm.

LGTM. The changes to DAGCombiner seem reasonable to me and I can see an improvement in the X86 code too.

llvm/lib/Target/RISCV/RISCVInstrInfoD.td
234

nit: If you still plan to go ahead with the patch could you add a brief comment as suggested?

This revision is now accepted and ready to land.Jan 26 2021, 12:31 AM
luismarques abandoned this revision.EditedThu, Feb 4, 7:33 AM

LGTM. The changes to DAGCombiner seem reasonable to me and I can see an improvement in the X86 code too.

This patch was originally intended to address FCOPYSIGN issues that no longer exist (in such a significant way). It also would need some tweaks when rebasing to avoid codegen regressions on new test cases, ones that arguably wouldn't be very clean. Furthermore, I believe @efriedma had argued that these legalization issues shouldn't be handled in the combiner. So, I've extracted the beneficial changes into a separate patch, D96037, and added you as a reviewer. I'm abandoning this patch.