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

[craig.topper]

Summary

I don't think we have any reason to believe the FP_ROUND here doesn't change the value.

Found while trying to see if we still need the fp128 block in CanCombineFCOPYSIGN_EXTEND_ROUND.
Removing that check caused this FP_ROUND to fire for fp128 which introduced a libcall expansion that asserted for this being a 1.

[pengfei]

I don't think we have any reason to believe the FP_ROUND here doesn't change the value.

But I think the change here doesn't matter since we clear all non sign bits later.

llvm/lib/Target/X86/X86ISelLowering.cpp

21903–21904

Better change the format as Lint suggested.

[craig.topper]

clang-format

[pengfei]

LGTM. But if there's benefit for optimization if we use 1 here?

[craig.topper]

In 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.

[pengfei]

In D96098#2544246, @craig.topper wrote:

In 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.

[craig.topper]

In D96098#2544279, @pengfei wrote:

In D96098#2544246, @craig.topper wrote:

In 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, @pengfei wrote:

In D96098#2544246, @craig.topper wrote:

In 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

[RKSimon]

LGTM but I'd be happier if you can add a test case

[craig.topper]

In D96098#2545540, @RKSimon wrote:

LGTM but I'd be happier if you can add a test case

The use of that flag that I know of is to combine (fpext (fpround X, 1)) -> X if the type doesn't change. Since the FP_ROUND is being consumed by an expanded FCOPYSIGN after this, there won't be a fpext consuming it. Can you think of any other places it is used?

[RKSimon]

In D96098#2545584, @craig.topper wrote:

In D96098#2545540, @RKSimon wrote:

LGTM but I'd be happier if you can add a test case

The use of that flag that I know of is to combine (fpext (fpround X, 1)) -> X if the type doesn't change. Since the FP_ROUND is being consumed by an expanded FCOPYSIGN after this, there won't be a fpext consuming it. Can you think of any other places it is used?

No sorry @spatel do you know ?

I'm OK for the patch to go without a test tbh.

[spatel]

In D96098#2546745, @RKSimon wrote:

In D96098#2545584, @craig.topper wrote:

In D96098#2545540, @RKSimon wrote:

LGTM but I'd be happier if you can add a test case

The use of that flag that I know of is to combine (fpext (fpround X, 1)) -> X if the type doesn't change. Since the FP_ROUND is being consumed by an expanded FCOPYSIGN after this, there won't be a fpext consuming it. Can you think of any other places it is used?

No sorry @spatel do you know ?

No, I don't know the history either.
I don't see any other uses of that extra param, so it might be possible to drop it completely? A potential complication is that we don't have fast-math-flags on all FP casts, so there may be some FP narrowing optimizations that are missed.

llvm/lib/Target/X86/X86ISelLowering.cpp

static SDValue LowerFCOPYSIGN(SDValue Op, SelectionDAG &DAG) {
 
   // If the sign operand is smaller, extend it first.
   MVT VT = Op.getSimpleValueType();
   if (Sign.getSimpleValueType().bitsLT(VT))
     Sign = DAG.getNode(ISD::FP_EXTEND, dl, VT, Sign);
 
   // And if it is bigger, shrink it first.
   if (Sign.getSimpleValueType().bitsGT(VT))
-    Sign = DAG.getNode(ISD::FP_ROUND, dl, VT, Sign, DAG.getIntPtrConstant(1, dl));
+    Sign =
+        DAG.getNode(ISD::FP_ROUND, dl, VT, Sign, DAG.getIntPtrConstant(0, dl));
 
   // At this point the operands and the result should have the same
   // type, and that won't be f80 since that is not custom lowered.
   bool IsF128 = (VT == MVT::f128);
   assert((VT == MVT::f64 || VT == MVT::f32 || VT == MVT::f128 ||
           VT == MVT::v2f64 || VT == MVT::v4f64 || VT == MVT::v4f32 ||
           VT == MVT::v8f32 || VT == MVT::v8f64 || VT == MVT::v16f32) &&
          "Unexpected type in LowerFCOPYSIGN");