[PATCH] D96098: [X86] Don't pass a 1 to the secon argument of ISD::FP_ROUND in LowerFCOPYSIGN.

Fri Feb 5 00:42:04 PST 2021

craig.topper added a comment.

In D96098#2544279 <https://reviews.llvm.org/D96098#2544279>, @pengfei wrote:

> In D96098#2544246 <https://reviews.llvm.org/D96098#2544246>, @craig.topper wrote:
>
>> In D96098#2544235 <https://reviews.llvm.org/D96098#2544235>, @pengfei wrote:
>>
>>> LGTM. But if there's benefit for optimization if we use 1 here?
>>
>> We're reconstructing an FP_ROUND that was removed by DAGCombine:visitFCOPYSIGN. DAGCombine didn't check the second operand before doing the combine. So we have to do the conservative thing when reconstructing it. I'm not sure how to retain the information unless we also add the operand to FCOPYSIGN.
>
> Then why we need to do these FP_ROUND/FP_EXTEND things. I didn't find any cases in LangRef and tests that the type of Sign is different from Mag or result.

The types are only allowed to be different in SelectionDAG not in IR.

In D96098#2544279 <https://reviews.llvm.org/D96098#2544279>, @pengfei wrote:

> In D96098#2544246 <https://reviews.llvm.org/D96098#2544246>, @craig.topper wrote:
>
>> In D96098#2544235 <https://reviews.llvm.org/D96098#2544235>, @pengfei wrote:
>>
>>> LGTM. But if there's benefit for optimization if we use 1 here?
>>
>> We're reconstructing an FP_ROUND that was removed by DAGCombine:visitFCOPYSIGN. DAGCombine didn't check the second operand before doing the combine. So we have to do the conservative thing when reconstructing it. I'm not sure how to retain the information unless we also add the operand to FCOPYSIGN.
>
> Then why we need to do these FP_ROUND/FP_EXTEND things. I didn't find any cases in LangRef and tests that the type of Sign is different from Mag or result.

It’s specific to SelectionDAG. Not sure the complete history of why it exists. It causes some other issues. You can trace back through other reviews from https://reviews.llvm.org/D96037

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D96098/new/

https://reviews.llvm.org/D96098