[legalize-types] Clean up softening machinery.

[ayartsev]

Summary

Hi all,

The attached patch is a replacement for D29265. It gets rid of special treating of SoftenedFloats and normalizes softening (makes it's logic similar to promotion, expansion, e.t.c). The patch is an effort to simplify and clarify legalization and to make perhaps the first step towards using a single map for all replacements (the idea from D29265).

Please review.

[chh]

I assume that this change is a clean up without functionality impact.
We should test this with Android code for the x86_64 f128 types.
I could do that later.

[chh]

lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp

115 ↗

(On Diff #94832)

Was there a problem to fix with the new condition (R.getNode() != N)?
I vaguely remember that x86_64 f128 type implementation might depend on the ability to set a note as softened without changing the node.

813 ↗

(On Diff #94832)

Why not skipping CopyToReg now?
If it missed some case, could you add a test case?

[chh]

c.zip15 KBDownload

The attached c.zip contains a test program s_fdim-45daeb.c,
generated from Android bionic/libm/upstream-freebsd/lib/msun/src/s_fdim.c
The compilation command in c.sh failed with this patch.
Clang aborted with the following message:

fatal error: error in backend: Cannot select: t7: f128 = fsub t2, t4
  t2: f128,ch = CopyFromReg t0, Register:f128 %vreg2
    t1: f128 = Register %vreg2
  t4: f128,ch = CopyFromReg t0, Register:f128 %vreg3
    t3: f128 = Register %vreg3
In function: fdiml

I think the change to COPY_TO_REG is causing the error.

[ayartsev]

Thanks for the review.
Addressed the issue, updated the patch.

lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp

115 ↗

(On Diff #94832)

No problems arose. In case when a softened node is not in the SoftenedFloats map GetSoftenedFloat() just returns the key node.

813 ↗

(On Diff #94832)

CopyToReg is now handled in SoftenFloatOperand(). CanSkipSoftenFloatOperand() only handles opcodes that were not handled by SoftenFloatOperand().

[chh]

Does the new patch work with my previous attached c.zip?
I don't see new changes related to CopyToReg.

Again I worry about the new changes to FABS, FCOPYSIGN, and SELECT.
Why do you need those changes, but not FNEG, Register, etc.?
Those op codes used to cause problems with Android x86_64 fp128 types.
That's why I have them and some other opcodes in the
CanSkipSoftenFloatOperand function.

Does the new test soft-fp-legal-in-HW-reg.ll catch any error?
It should have some CHECK tags to verify compiled output.
It should also be tested with the android x86_64 mode, like:

RUN: llc < %s -O2 -mtriple=x86_64-linux-android -mattr=+mmx \
RUN:     -enable-legalize-types-checking | FileCheck %s --check-prefix=MMX
RUN: llc < %s -O2 -mtriple=x86_64-linux-gnu -mattr=+mmx \
RUN:     -enable-legalize-types-checking | FileCheck %s --check-prefix=MMX

Please see other tests in fp128-*.ll.
Thanks.

[ayartsev]

Does the new patch work with my previous attached c.zip?
I don't see new changes related to CopyToReg.

I've managed to reproduce the error from your attached c.zip. Here is the minimal test derived from the fdiml() implementation (the origin of the error as seen from the error message):

long double fn(long double x, long double y) {
  return (x > y ? x - y : 0.0);
}

The problem was not related to CopyToReg but to handling of ISD::SELECT. Before this patch operands of ISD::CopyToReg, ISD::SELECT, etc. were implicitly replaced by ReplaceSoftenFloatResult() called from SoftenFloatResult(). Now operands are explicitly replaced by their softened versions (if any) in SoftenFloatOperand().

Again I worry about the new changes to FABS, FCOPYSIGN, and SELECT.
Why do you need those changes, but not FNEG, Register, etc.?
Those op codes used to cause problems with Android x86_64 fp128 types.
That's why I have them and some other opcodes in the
CanSkipSoftenFloatOperand function.

Operands of CopyToReg, FABS, FCOPYSIGN, and SELECT may have been softened and must be replaced by SoftenFloatOperand() as I mentioned above. As for other opcodes - FNEG, Register, etc. - I'm currently failing to write regression tests for this opcodes. It may be impossible to have softened operands for this opcodes.

Does the new test soft-fp-legal-in-HW-reg.ll catch any error?
It should have some CHECK tags to verify compiled output.

The new test checks that stderr is empty and we don't end up with "fatal error: Cannot select:...". Similar tests exist among LLVM tests, look at \test\CodeGen\X86\xmm-r64.ll for example.

It should also be tested with the android x86_64 mode

Tested with -mtriple=x86_64-linux-android. The result remains the same. The patch cause no regression in LLVM tests. Do you have a chance to test the new patch over Android codebase?

Thanks for the feedback!

[chh]

Maybe some problem with CopyToReg is fixed with new changes to SELECT,
but there are other errors I found from Android libm.
Please try the attached x.zip, which has three preprocessed files that
this new patch will fail to compile:

/tmp/cpowl-3f7b0a.c

fatal error: error in backend: Cannot select: t5: f128 = fmul t2, t4

t2: f128,ch = CopyFromReg t0, Register:f128 %vreg1
  t1: f128 = Register %vreg1
t4: f128,ch = CopyFromReg t0, Register:f128 %vreg5
  t3: f128 = Register %vreg5

In function: cpowl

/tmp/ccosl-afde3f.c

fatal error: error in backend: Cannot select: t53: f128 = fmul t51, t80

t51: f128,ch,glue = CopyFromReg t50, Register:f128 %XMM0, t50:1
  t25: f128 = Register %XMM0
  t50: ch,glue = callseq_end t49, TargetConstant:i64<0>, TargetConstant:i64<0>, t49:1
    t23: i64 = TargetConstant<0>
    t23: i64 = TargetConstant<0>
    t49: ch,glue = X86ISD::CALL t47, TargetGlobalAddress:i64<fp128 (fp128)* @sinl> 0 [TF=6], Register:f128 %XMM0, RegisterMask:Untyped, t47:1
      t48: i64 = TargetGlobalAddress<fp128 (fp128)* @sinl> 0 [TF=6]
      t25: f128 = Register %XMM0
      t32: Untyped = RegisterMask
      t47: ch,glue = CopyToReg t46, Register:f128 %XMM0, t35
        t25: f128 = Register %XMM0
        t35: f128,ch = load<LD16[%9]> t34, FrameIndex:i64<-1>, undef:i64
          t3: i64 = FrameIndex<-1>
          t9: i64 = undef
t80: f128,ch = load<LD16[%6](dereferenceable)> t50, FrameIndex:i64<2>, undef:i64
  t16: i64 = FrameIndex<2>
  t9: i64 = undef

In function: ccosl

/tmp/e_hypotl-9d5113.c

fatal error: error in backend: Cannot select: t20: f128 = fmul t19, t19

t19: f128,ch = CopyFromReg t0, Register:f128 %vreg32
  t18: f128 = Register %vreg32
t19: f128,ch = CopyFromReg t0, Register:f128 %vreg32
  t18: f128 = Register %vreg32

In function: hypotl

I will be on vacation for about 1 week.
Probably won't be able to help you test new changes before I come back.

Please note that even if clang does not abort, like your test case show,
we should check if the output code actually put fp128 values in SSE registers
as before. It's not just an optimization but a required calling convention in
many cases.

x.zip62 KBDownload

[ayartsev]

Great! Here ISD::FNEG caused the problem at least in two tests. Updated the patch. Hope this was the last regression.

Please note that even if clang does not abort, like your test case show,
we should check if the output code actually put fp128 values in SSE registers
as before.

Done.
Compared the output code generated from the soft-fp-legal-in-HW-reg.ll before and after the patch - the code is the same.

[chh]

Before making more changes and running more tests,
could you update the subject/summary or rethink purpose of this change?
I still don't understand all the reasons and scope.

(1) What is the "single map"?
I don't see elimination or unification of any map.

(2) It is not easy to see that all these changes to SoftenedFloats
are required to replace D29265.

D29265 was a simple change to remove stale entries to pass expensive checks.
The following change in SoftenFloatResult seems to be the replacement:

do not call SetSoftenedFloat(SDValue(N, ResNo), R) when (R.getNode() == N).

The move of some opcode out of CanSkipSoftenFloatOperand seems unrelated.

If SetSoftenedFloat(SDValue(N, ResNo), R) is not called, and no "stale"
entry is created, why do we need the code pattern in new SoftenFloatOp_*?
It says in general:

if (GetSoftenedFloat(operand) == operand)
  return SDValue();

When can (GetSoftenedFloat(operand) == operand) happen?
I know that the pattern must be needed or some test case will fail,
but it is just difficult to understand without each test case.

IMHO, I think it is easier to understand if we allow

SetSoftenedFloat(SDValue(N, ResNo), R) when (R.getNode() == N),

and

(GetSoftenedFloat(operand) == operand).

Then, the problem we have is just some "redundant" or "stale" entries
that can be easily removed by D29265.

(3) For whatever reason to change CanSkipSoftenFloatOperand,
why only some but not all cases are moved out of CanSkipSoftenFloatOperand?
It's difficult to prove that ISD::Register, ISD::ConstantFP,
and ISD::CopyFromReg do not deserve the same treatment.

Well, I'm still thinking of not changing CanSkipSoftenFloatOperand at all.
The new change requires more computation and code, and the benefit is not
seen yet without the "use a single map".

[ayartsev]

If you look at other legalization functions (ExpandFloatResult/Operand, PromoteIntegerResult/Operand, e.t.c) you can see that they all share the same logic in node processing: functions, that handle node results (e.g. ExpandFloatResult) perform their actions and place the resulting node to the corresponding map (e.g. ExpandFloats) for further use, while functions that process operands (e.g. ExpandFloatOperand) perform their action and replace the old nodes with the new ones (that is update the ReplacedValues map).
It is also seen from the code that for a given node legalization of node results preceed processing of this node as operand of another node, so values from maps like SoftenFloats (placed there by result legalizing routines) will be used for replacement/subtitution, if needed, by operand processing routines in the fullness of time.
SoftenFloatResult() routine design breaks this scheme - it puts a result to the SoftenFloats map AND replaces this result in place by calling ReplaceSoftenFloatResult() for dubious benefit. It forces us to specially remember and skip all this replacement cases in SoftenFloatOperand() by calling CanSkipSoftenFloatOperand(). It also makes a key present both in SoftenFloats and ReplacedValues maps under some circumstances that expensive check detects. It took an effort to understand whats going on. D29265 doesn't fix the problem with the same value in multiple maps and just makes the code more unclear.
Actually, softening really slightly differs from other legalization routines, its trait is that nothing should be done with the node result (except ISD::LOAD) if the result can be kept in HW registers. This trait gave the idea for, IMHO, unsuccessful improvement with replacing a result in place by calling ReplaceSoftenFloatResult().
So my patch makes SoftenFloatResult/Operand logic just the same as all other legalization routines have: SoftenFloatResult() fills the SoftenFloats map and SoftenFloatOperand() perform all needed replacements. With the special case for values, that can be kept in HW registers - in this case SoftenFloatResult() just does nothing. I intended to get rid of calling SoftenFloatResult() for HW suitable values at all, but ISD::LOAD creates a precedent. I've also analyzed all usages of the SoftenFloats map and didn't find any need for holding values that map to itself.
Hope this answers most of your questions.

(1) What is the "single map"?
I don't see elimination or unification of any map.

I ment unification of the node processing logic as described above.

D29265 was a simple change to remove stale entries to pass expensive checks.
The following change in SoftenFloatResult seems to be the replacement:

do not call SetSoftenedFloat(SDValue(N, ResNo), R) when (R.getNode() == N).

At first I've made ReplaceSoftenFloatResult() return 'true' in case if it performes a replacement and to return 'false' otherwise and tried many different combinations of conditions here and each one caused the regression in some spacial cases and forced me to complicate the logic. This experiments led me to understanding that this is not the right place to fix the problem.

If SetSoftenedFloat(SDValue(N, ResNo), R) is not called, and no "stale"
entry is created, why do we need the code pattern in new SoftenFloatOp_*?
It says in general:

if (GetSoftenedFloat(operand) == operand)
return SDValue();

When can (GetSoftenedFloat(operand) == operand) happen?

(GetSoftenedFloat(operand) == operand) mean that SoftenFloatResult() left this 'operand' node results unchanged (because of legality in HW) and there is nothing to do with this node here in SoftenFloatOperand() so we return SDValue() to tell that the node no need to be processed.

(3) For whatever reason to change CanSkipSoftenFloatOperand,
why only some but not all cases are moved out of CanSkipSoftenFloatOperand?
It's difficult to prove that ISD::Register, ISD::ConstantFP,
and ISD::CopyFromReg do not deserve the same treatment.

Yes, you are right, all this cases should be also moved out of CanSkipSoftenFloatOperand(). I'll do this in the new patch in case you accept my solution.

[chh]

I think now I understand your goal, which still seems not exactly
explained by "use a single map for replacements".

Please take a look of my patches
https://reviews.llvm.org/D15134
https://reviews.llvm.org/D11438
and their review history.

Current code is a compromise of various reviewer comments and
to avoid larger risky changes of the existing legalization process.
When I worked on those patches I also wished to have some clean up
of the legalization and type "soften" process. Other reviewers also
have suggested what you are trying to do now, but we never have
found a simpler and low risk change.

The difficulty was due to our limited knowledge of the
current complicated "type legalization" code,
and our limited resource to test on all targets.

I tested this patch against Android open source today and it failed to
compile 15 source files in bionic/libm/upstream-freebsd/lib/msun/src
for the x86_64 target. We need more consistent changes to all those
switch cases, to have a better chance to avoid errors. Still, it is
quite time consuming to go through one Android build test.

From those 15 failed cases,
I copy only 3 instruction selection fatal errors like:

bionic/libc/upstream-openbsd/lib/libc/gdtoa/hdtoa.c

fatal error: error in backend: Cannot select: 0x5d43570: i32 = truncate 0x5d43a50

0x5d43a50: f128 = srl 0x5d48468, Constant:i8<112>
  0x5d48468: f128,ch,glue = CopyFromReg 0x5d48c20, Register:f128 %XMM0, 0x5d48c20:1
    0x5d48878: f128 = Register %XMM0
    0x5d48c20: ch,glue = callseq_end 0x5d43368, TargetConstant:i64<0>, TargetConstant:i64<0>, 0x5d43368:1
      0x5d488e0: i64 = TargetConstant<0>
      0x5d488e0: i64 = TargetConstant<0>
      0x5d43368: ch,glue = X86ISD::CALL 0x5d43c58, TargetExternalSymbol:i64'__multf3' [TF=6], Register:f128 %XMM0, Register:f128 %XMM1, RegisterMask:Untyped, 0x5d43c58:1
        0x5d48a80: i64 = TargetExternalSymbol'__multf3' [TF=6]
        0x5d48878: f128 = Register %XMM0
        0x5d50758: f128 = Register %XMM1
        0x5d44270: Untyped = RegisterMask
        0x5d43c58: ch,glue = CopyToReg 0x5d48810, Register:f128 %XMM1, 0x5d49030, 0x5d48810:1
          0x5d50758: f128 = Register %XMM1
          0x5d49030: f128,ch = load<LD16[ConstantPool]> 0x5c5f028, 0x5d49238, undef:i64
            0x5d49238: i64 = X86ISD::WrapperRIP TargetConstantPool:i64<fp128 0xL00000000000000004201000000000000> 0
              0x5d440d0: i64 = TargetConstantPool<fp128 0xL00000000000000004201000000000000> 0
            0x5d50620: i64 = undef
          0x5d48810: ch,glue = CopyToReg 0x5d43300, Register:f128 %XMM0, 0x5d43d90
            0x5d48878: f128 = Register %XMM0
            0x5d43d90: f128,ch = CopyFromReg 0x5c5f028, Register:f128 %vreg108
              0x5d48dc0: f128 = Register %vreg108
  0x5d48fc8: i8 = Constant<112>

In function: __hldtoa

bionic/libm/upstream-freebsd/lib/msun/src/s_cbrtl.c
fatal error: error in backend: Cannot select: 0x3ed5838: i64 = truncate 0x3ed5b10

0x3ed5b10: f128 = srl 0x3ed5de8, Constant:i8<64>
  0x3ed5de8: f128,ch,glue = CopyFromReg 0x3ed6058, Register:f128 %XMM0, 0x3ed6058:1
    0x3eda090: f128 = Register %XMM0
    0x3ed6058: ch,glue = callseq_end 0x3ed6128, TargetConstant:i64<0>, TargetConstant:i64<0>, 0x3ed6128:1
      0x3eda028: i64 = TargetConstant<0>
      0x3eda028: i64 = TargetConstant<0>
      0x3ed6128: ch,glue = X86ISD::CALL 0x3ed6398, TargetExternalSymbol:i64'__multf3' [TF=6], Register:f128 %XMM0, Register:f128 %XMM1, RegisterMask:Untyped, 0x3ed6398:1
        0x3ed5d80: i64 = TargetExternalSymbol'__multf3' [TF=6]
        0x3eda090: f128 = Register %XMM0
        0x3ed9fc0: f128 = Register %XMM1
        0x3ed54f8: Untyped = RegisterMask
        0x3ed6398: ch,glue = CopyToReg 0x3ed6400, Register:f128 %XMM1, 0x3ed5490, 0x3ed6400:1
          0x3ed9fc0: f128 = Register %XMM1
          0x3ed5490: f128,ch = load<LD16[ConstantPool]> 0x3e2ed08, 0x3ed5970, undef:i64
            0x3ed5970: i64 = X86ISD::WrapperRIP TargetConstantPool:i64<fp128 0xL00000000000000004201000000000000> 0
              0x3ed5560: i64 = TargetConstantPool<fp128 0xL00000000000000004201000000000000> 0
            0x3ed5b78: i64 = undef
          0x3ed6400: ch,glue = CopyToReg 0x3ed57d0, Register:f128 %XMM0, 0x3ed5700
            0x3eda090: f128 = Register %XMM0
            0x3ed5700: f128,ch = CopyFromReg 0x3e2ed08, Register:f128 %vreg21
              0x3eda160: f128 = Register %vreg21
  0x3eda0f8: i8 = Constant<64>

In function: cbrtl

bionic/libm/upstream-freebsd/lib/msun/src/e_hypotl.c
fatal error: error in backend: Cannot select: 0x44159a0: ch = store<ST8[FixedStack11+8], trunc to i64> 0x4368b88, 0x44197d0, 0x4415868, undef:i64

0x44197d0: f128 = srl 0x4415590, Constant:i8<64>
  0x4415590: f128,ch,glue = CopyFromReg 0x44151e8, Register:f128 %XMM0, 0x44151e8:1
    0x44156c8: f128 = Register %XMM0
    0x44151e8: ch,glue = callseq_end 0x44198a0, TargetConstant:i64<0>, TargetConstant:i64<0>, 0x44198a0:1
      0x4419d18: i64 = TargetConstant<0>
      0x4419d18: i64 = TargetConstant<0>
      0x44198a0: ch,glue = X86ISD::CALL 0x44150b0, TargetExternalSymbol:i64'__addtf3' [TF=6], Register:f128 %XMM0, Register:f128 %XMM1, RegisterMask:Untyped, 0x44150b0:1
        0x4419ff0: i64 = TargetExternalSymbol'__addtf3' [TF=6]
        0x44156c8: f128 = Register %XMM0
        0x4415800: f128 = Register %XMM1
        0x4414f78: Untyped = RegisterMask
        0x44150b0: ch,glue = CopyToReg 0x4419b10, Register:f128 %XMM1, 0x441a400, 0x4419b10:1
          0x4415800: f128 = Register %XMM1
          0x441a400: f128,ch = CopyFromReg 0x4368b88, Register:f128 %vreg25
            0x4419b78: f128 = Register %vreg25
          0x4419b10: ch,glue = CopyToReg 0x441a398, Register:f128 %XMM0, 0x441a400
            0x44156c8: f128 = Register %XMM0
            0x441a400: f128,ch = CopyFromReg 0x4368b88, Register:f128 %vreg25
              0x4419b78: f128 = Register %vreg25
  0x4415798: i8 = Constant<64>
0x4415868: i64 = or FrameIndex:i64<11>, Constant:i64<8>
  0x441a4d0: i64 = FrameIndex<11>
  0x4414c38: i64 = Constant<8>
0x4414ca0: i64 = undef

In function: hypotl

[ayartsev]

Updated the patch, changed the title.

Please take a look of my patches
https://reviews.llvm.org/D15134
https://reviews.llvm.org/D11438
and their review history.

I looked through your patches and review history, great job! I'm feeling on the right way.

I copy only 3 instruction selection fatal errors like:

Failing to reproduce either of them, in my case the compilation succeed and I'm getting DAGs different from shown in error dumps. I've used command line args from 'c.sh' from your attach 'c.zip' above and preprocessed 'e_hypotl-9d5113.c' from your attach 'x.zip' (other two sources 'hdtoa.c' and 's_cbrtl.c' I've prepared myself based on the 'e_hypotl-9d5113.c').

What confuses me is that all this sources fail on the "f128 = srl 0x......., Constant:i8<64>" instruction, that takes and returns 'f128' type, but I've failed to invent a test to make codegen emit 'srl' instruction with float argument/return value. Neither test in LLVM codebase has such an example. Neither my output DAGs for failing tests had this float 'srl'. Maybe you know, how to make codegen emit f128' srl? Or could you, please, test the new patch one more time and, if problems persist, send me the script with Clang command line for the failing source and the preprocessed source itself?
Sorry for the long delay and thank you for looking at this.

[chh]

Still running some Android tests.
This new diff looks good.
I only found one error in soft-fp-legal-in-HW-reg.ll so far.

test/CodeGen/X86/soft-fp-legal-in-HW-reg.ll

2 ↗

(On Diff #104555)

s/RUN/RUN:/

[chh]

Except the unit test failure, I did not find error in building Android.
Please fix the unit test before submit.
I probably won't have time working on this before next Wednesday.

llvm/trunk/lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp

#endif
     case ISD::SELECT:      R = SoftenFloatRes_SELECT(N, ResNo); break;
     case ISD::SELECT_CC:   R = SoftenFloatRes_SELECT_CC(N, ResNo); break;
     case ISD::SINT_TO_FP:
     case ISD::UINT_TO_FP:  R = SoftenFloatRes_XINT_TO_FP(N); break;
     case ISD::UNDEF:       R = SoftenFloatRes_UNDEF(N); break;
     case ISD::VAARG:       R = SoftenFloatRes_VAARG(N); break;
   }
 
-  // If R is null, the sub-method took care of registering the result.
-  if (R.getNode()) {
+  if (R.getNode() && R.getNode() != N) {
     SetSoftenedFloat(SDValue(N, ResNo), R);
-    ReplaceSoftenFloatResult(N, ResNo, R);
+    // Return true only if the node is changed, assuming that the operands
+    // are also converted when necessary.
+    return true;
   }
-  // Return true only if the node is changed,
-  // assuming that the operands are also converted when necessary.
+
   // Otherwise, return false to tell caller to scan operands.
-  return R.getNode() && R.getNode() != N;
+  return false;
 }
 
 SDValue DAGTypeLegalizer::SoftenFloatRes_BITCAST(SDNode *N, unsigned ResNo) {
   if (isLegalInHWReg(N->getValueType(ResNo)))
     return SDValue(N, ResNo);
   return BitConvertToInteger(N->getOperand(0));
 }
 
if (CanSkipSoftenFloatOperand(N, OpNo))
       return false;
 #ifndef NDEBUG
     dbgs() << "SoftenFloatOperand Op #" << OpNo << ": ";
     N->dump(&DAG); dbgs() << "\n";
 #endif
     llvm_unreachable("Do not know how to soften this operator's operand!");
 
   case ISD::BITCAST:     Res = SoftenFloatOp_BITCAST(N); break;
+  case ISD::CopyToReg:   Res = SoftenFloatOp_COPY_TO_REG(N); break;
   case ISD::BR_CC:       Res = SoftenFloatOp_BR_CC(N); break;
+  case ISD::FABS:        Res = SoftenFloatOp_FABS(N); break;
+  case ISD::FCOPYSIGN:   Res = SoftenFloatOp_FCOPYSIGN(N); break;
+  case ISD::FNEG:        Res = SoftenFloatOp_FNEG(N); break;
   case ISD::FP_EXTEND:   Res = SoftenFloatOp_FP_EXTEND(N); break;
   case ISD::FP_TO_FP16:  // Same as FP_ROUND for softening purposes
   case ISD::FP_ROUND:    Res = SoftenFloatOp_FP_ROUND(N); break;
   case ISD::FP_TO_SINT:
   case ISD::FP_TO_UINT:  Res = SoftenFloatOp_FP_TO_XINT(N); break;
+  case ISD::SELECT:      Res = SoftenFloatOp_SELECT(N); break;
   case ISD::SELECT_CC:   Res = SoftenFloatOp_SELECT_CC(N); break;
   case ISD::SETCC:       Res = SoftenFloatOp_SETCC(N); break;
   case ISD::STORE:
     Res = SoftenFloatOp_STORE(N, OpNo);
     // Do not try to analyze or soften this node again if the value is
     // or can be held in a register. In that case, Res.getNode() should
     // be equal to N.
     if (Res.getNode() == N &&
#endif
 
   ReplaceValueWith(SDValue(N, 0), Res);
   return false;
 }
 
 bool DAGTypeLegalizer::CanSkipSoftenFloatOperand(SDNode *N, unsigned OpNo) {
   if (!isLegalInHWReg(N->getOperand(OpNo).getValueType()))
     return false;
-  // When the operand type can be kept in registers, SoftenFloatResult
-  // will call ReplaceValueWith to replace all references and we can
-  // skip softening this operand.
+
+  // When the operand type can be kept in registers there is nothing to do for
+  // the following opcodes.
   switch (N->getOperand(OpNo).getOpcode()) {
     case ISD::BITCAST:
     case ISD::ConstantFP:
     case ISD::CopyFromReg:
     case ISD::CopyToReg:
     case ISD::FABS:
     case ISD::FCOPYSIGN:
     case ISD::FNEG:
     case ISD::Register:
     case ISD::SELECT:
     case ISD::SELECT_CC:
       return true;
   }
-  // For some opcodes, SoftenFloatResult handles all conversion of softening
-  // and replacing operands, so that there is no need to soften operands
-  // again, although such opcode could be scanned for other illegal operands.
+
   switch (N->getOpcode()) {
-    case ISD::ConstantFP:
-    case ISD::CopyFromReg:
-    case ISD::CopyToReg:
-    case ISD::FABS:
-    case ISD::FCOPYSIGN:
-    case ISD::FNEG:
-    case ISD::Register:
-    case ISD::SELECT:
+    case ISD::ConstantFP:  // Leaf node.
+    case ISD::CopyFromReg: // Operand is a register that we know to be left 
+                           // unchanged by SoftenFloatResult().
+    case ISD::Register:    // Leaf node.
       return true;
   }
   return false;
 }
 
 SDValue DAGTypeLegalizer::SoftenFloatOp_BITCAST(SDNode *N) {
   return DAG.getNode(ISD::BITCAST, SDLoc(N), N->getValueType(0),
                      GetSoftenedFloat(N->getOperand(0)));
 }
 
+SDValue DAGTypeLegalizer::SoftenFloatOp_COPY_TO_REG(SDNode *N) {
+  SDValue Op1 = GetSoftenedFloat(N->getOperand(1));
+  SDValue Op2 = GetSoftenedFloat(N->getOperand(2));
+
+  if (Op1 == N->getOperand(1) && Op2 == N->getOperand(2))
+    return SDValue();
+
+  if (N->getNumOperands() == 3)
+    return SDValue(DAG.UpdateNodeOperands(N, N->getOperand(0), Op1, Op2), 0);
+
+  return SDValue(DAG.UpdateNodeOperands(N, N->getOperand(0), Op1, Op2, 
+                                        N->getOperand(3)),
+                 0);
+}
+
 SDValue DAGTypeLegalizer::SoftenFloatOp_FP_EXTEND(SDNode *N) {
   // If we get here, the result must be legal but the source illegal.
   EVT SVT = N->getOperand(0).getValueType();
   EVT RVT = N->getValueType(0);
   SDValue Op = GetSoftenedFloat(N->getOperand(0));
 
   if (SVT == MVT::f16)
     return DAG.getNode(ISD::FP16_TO_FP, SDLoc(N), RVT, Op);
SDValue DAGTypeLegalizer::SoftenFloatOp_BR_CC(SDNode *N) {
 
   // Update N to have the operands specified.
   return SDValue(DAG.UpdateNodeOperands(N, N->getOperand(0),
                                 DAG.getCondCode(CCCode), NewLHS, NewRHS,
                                 N->getOperand(4)),
                  0);
 }
 
+SDValue DAGTypeLegalizer::SoftenFloatOp_FABS(SDNode *N) {
+  SDValue Op = GetSoftenedFloat(N->getOperand(0));
+
+  if (Op == N->getOperand(0))
+    return SDValue();
+
+  return SDValue(DAG.UpdateNodeOperands(N, Op), 0);
+}
+
+SDValue DAGTypeLegalizer::SoftenFloatOp_FCOPYSIGN(SDNode *N) {
+  SDValue Op0 = GetSoftenedFloat(N->getOperand(0));
+  SDValue Op1 = GetSoftenedFloat(N->getOperand(1));
+
+  if (Op0 == N->getOperand(0) && Op1 == N->getOperand(1))
+    return SDValue();
+
+  return SDValue(DAG.UpdateNodeOperands(N, Op0, Op1), 0);
+}
+
+SDValue DAGTypeLegalizer::SoftenFloatOp_FNEG(SDNode *N) {
+  SDValue Op = GetSoftenedFloat(N->getOperand(0));
+
+  if (Op == N->getOperand(0))
+    return SDValue();
+
+  return SDValue(DAG.UpdateNodeOperands(N, Op), 0);
+}
+
 SDValue DAGTypeLegalizer::SoftenFloatOp_FP_TO_XINT(SDNode *N) {
   bool Signed = N->getOpcode() == ISD::FP_TO_SINT;
   EVT SVT = N->getOperand(0).getValueType();
   EVT RVT = N->getValueType(0);
   EVT NVT = EVT();
   SDLoc dl(N);
 
   // If the result is not legal, eg: fp -> i1, then it needs to be promoted to
SDValue DAGTypeLegalizer::SoftenFloatOp_FP_TO_XINT(SDNode *N) {
 
   SDValue Op = GetSoftenedFloat(N->getOperand(0));
   SDValue Res = TLI.makeLibCall(DAG, LC, NVT, Op, false, dl).first;
 
   // Truncate the result if the libcall returns a larger type.
   return DAG.getNode(ISD::TRUNCATE, dl, RVT, Res);
 }
 
+SDValue DAGTypeLegalizer::SoftenFloatOp_SELECT(SDNode *N) {
+  SDValue Op1 = GetSoftenedFloat(N->getOperand(1));
+  SDValue Op2 = GetSoftenedFloat(N->getOperand(2));
+
+  if (Op1 == N->getOperand(1) && Op2 == N->getOperand(2))
+    return SDValue();
+
+  return SDValue(DAG.UpdateNodeOperands(N, N->getOperand(0), Op1, Op2),
+                 0);
+}
+
 SDValue DAGTypeLegalizer::SoftenFloatOp_SELECT_CC(SDNode *N) {
   SDValue NewLHS = N->getOperand(0), NewRHS = N->getOperand(1);
   ISD::CondCode CCCode = cast<CondCodeSDNode>(N->getOperand(4))->get();
 
   EVT VT = NewLHS.getValueType();
   NewLHS = GetSoftenedFloat(NewLHS);
   NewRHS = GetSoftenedFloat(NewRHS);
   TLI.softenSetCCOperands(DAG, VT, NewLHS, NewRHS, CCCode, SDLoc(N));

llvm/trunk/lib/CodeGen/SelectionDAG/LegalizeTypes.h

if (!SoftenedOp.getNode() &&
         isSimpleLegalType(Op.getValueType()))
       return Op;
     RemapValue(SoftenedOp);
     assert(SoftenedOp.getNode() && "Operand wasn't converted to integer?");
     return SoftenedOp;
   }
   void SetSoftenedFloat(SDValue Op, SDValue Result);
 
-  // Call ReplaceValueWith(SDValue(N, ResNo), Res) if necessary.
-  void ReplaceSoftenFloatResult(SDNode *N, unsigned ResNo, SDValue &NewRes) {
-    // When the result type can be kept in HW registers, the converted
-    // NewRes node could have the same type. We can save the effort in
-    // cloning every user of N in SoftenFloatOperand or other legalization functions,
-    // by calling ReplaceValueWith here to update all users.
-    if (NewRes.getNode() != N && isLegalInHWReg(N->getValueType(ResNo)))
-      ReplaceValueWith(SDValue(N, ResNo), NewRes);
-  }
-
   // Convert Float Results to Integer for Non-HW-supported Operations.
   bool SoftenFloatResult(SDNode *N, unsigned ResNo);
   SDValue SoftenFloatRes_MERGE_VALUES(SDNode *N, unsigned ResNo);
   SDValue SoftenFloatRes_BITCAST(SDNode *N, unsigned ResNo);
   SDValue SoftenFloatRes_BUILD_PAIR(SDNode *N);
   SDValue SoftenFloatRes_ConstantFP(SDNode *N, unsigned ResNo);
   SDValue SoftenFloatRes_EXTRACT_VECTOR_ELT(SDNode *N, unsigned ResNo);
   SDValue SoftenFloatRes_FABS(SDNode *N, unsigned ResNo);
private:
   SDValue SoftenFloatRes_LOAD(SDNode *N, unsigned ResNo);
   SDValue SoftenFloatRes_SELECT(SDNode *N, unsigned ResNo);
   SDValue SoftenFloatRes_SELECT_CC(SDNode *N, unsigned ResNo);
   SDValue SoftenFloatRes_UNDEF(SDNode *N);
   SDValue SoftenFloatRes_VAARG(SDNode *N);
   SDValue SoftenFloatRes_XINT_TO_FP(SDNode *N);
 
   // Return true if we can skip softening the given operand or SDNode because
-  // it was soften before by SoftenFloatResult and references to the operand
-  // were replaced by ReplaceValueWith.
+  // either it was soften before by SoftenFloatResult and references to the 
+  // operand were replaced by ReplaceValueWith or it's value type is legal in HW
+  // registers and the operand can be left unchanged.
   bool CanSkipSoftenFloatOperand(SDNode *N, unsigned OpNo);
 
   // Convert Float Operand to Integer for Non-HW-supported Operations.
   bool SoftenFloatOperand(SDNode *N, unsigned OpNo);
   SDValue SoftenFloatOp_BITCAST(SDNode *N);
+  SDValue SoftenFloatOp_COPY_TO_REG(SDNode *N);
   SDValue SoftenFloatOp_BR_CC(SDNode *N);
+  SDValue SoftenFloatOp_FABS(SDNode *N);
+  SDValue SoftenFloatOp_FCOPYSIGN(SDNode *N);
+  SDValue SoftenFloatOp_FNEG(SDNode *N);
   SDValue SoftenFloatOp_FP_EXTEND(SDNode *N);
   SDValue SoftenFloatOp_FP_ROUND(SDNode *N);
   SDValue SoftenFloatOp_FP_TO_XINT(SDNode *N);
+  SDValue SoftenFloatOp_SELECT(SDNode *N);
   SDValue SoftenFloatOp_SELECT_CC(SDNode *N);
   SDValue SoftenFloatOp_SETCC(SDNode *N);
   SDValue SoftenFloatOp_STORE(SDNode *N, unsigned OpNo);
 
   //===--------------------------------------------------------------------===//
   // Float Expansion Support: LegalizeFloatTypes.cpp
   //===--------------------------------------------------------------------===//
 

llvm/trunk/lib/CodeGen/SelectionDAG/LegalizeTypes.cpp

void DAGTypeLegalizer::PerformExpensiveChecks() {
   SmallVector<SDNode*, 16> NewNodes;
   for (SDNode &Node : DAG.allnodes()) {
     // Remember nodes marked NewNode - they are subject to extra checking below.
     if (Node.getNodeId() == NewNode)
       NewNodes.push_back(&Node);
 
     for (unsigned i = 0, e = Node.getNumValues(); i != e; ++i) {
       SDValue Res(&Node, i);
+      EVT VT = Res.getValueType();
       bool Failed = false;
 
       unsigned Mapped = 0;
       if (ReplacedValues.find(Res) != ReplacedValues.end()) {
         Mapped |= 1;
         // Check that remapped values are only used by nodes marked NewNode.
         for (SDNode::use_iterator UI = Node.use_begin(), UE = Node.use_end();
              UI != UE; ++UI)
for (unsigned i = 0, e = Node.getNumValues(); i != e; ++i) {
         // Since we allow ReplacedValues to map deleted nodes, it may map nodes
         // marked NewNode too, since a deleted node may have been reallocated as
         // another node that has not been seen by the LegalizeTypes machinery.
         if ((Node.getNodeId() == NewNode && Mapped > 1) ||
             (Node.getNodeId() != NewNode && Mapped != 0)) {
           dbgs() << "Unprocessed value in a map!";
           Failed = true;
         }
-      } else if (isTypeLegal(Res.getValueType()) || IgnoreNodeResults(&Node)) {
+      } else if (isTypeLegal(VT) || IgnoreNodeResults(&Node)) {
         if (Mapped > 1) {
           dbgs() << "Value with legal type was transformed!";
           Failed = true;
         }
       } else {
-        if (Mapped == 0) {
+        // If the value can be kept in HW registers, softening machinery can
+        // leave it unchanged and don't put it to any map.
+        if (Mapped == 0 &&
+            !(getTypeAction(VT) == TargetLowering::TypeSoftenFloat &&
+              isLegalInHWReg(VT))) {
           dbgs() << "Processed value not in any map!";
           Failed = true;
         } else if (Mapped & (Mapped - 1)) {
           dbgs() << "Value in multiple maps!";
           Failed = true;
         }
       }
 
ScanOperands:
     }
 
     // The sub-method updated N in place.  Check to see if any operands are new,
     // and if so, mark them.  If the node needs revisiting, don't add all users
     // to the worklist etc.
     if (NeedsReanalyzing) {
       assert(N->getNodeId() == ReadyToProcess && "Node ID recalculated?");
 
-      // Remove any result values from SoftenedFloats as N will be revisited
-      // again.
-      for (unsigned i = 0, NumResults = N->getNumValues(); i < NumResults; ++i)
-        SoftenedFloats.erase(SDValue(N, i));
-
       N->setNodeId(NewNode);
       // Recompute the NodeId and correct processed operands, adding the node to
       // the worklist if ready.
       SDNode *M = AnalyzeNewNode(N);
       if (M == N)
         // The node didn't morph - nothing special to do, it will be revisited.
         continue;
 
while (!NodesToAnalyze.empty()) {
         }
         // The original node continues to exist in the DAG, marked NewNode.
       }
     }
     // When recursively update nodes with new nodes, it is possible to have
     // new uses of From due to CSE. If this happens, replace the new uses of
     // From with To.
   } while (!From.use_empty());
-
-  SoftenedFloats.erase(From);
 }
 
 void DAGTypeLegalizer::SetPromotedInteger(SDValue Op, SDValue Result) {
   assert(Result.getValueType() ==
          TLI.getTypeToTransformTo(*DAG.getContext(), Op.getValueType()) &&
          "Invalid type for promoted integer");
   AnalyzeNewValue(Result);
 

llvm/trunk/lib/CodeGen/SelectionDAG/LegalizeTypesGeneric.cpp

case TargetLowering::TypePromoteInteger:
       break;
     case TargetLowering::TypePromoteFloat:
       llvm_unreachable("Bitcast of a promotion-needing float should never need"
                        "expansion");
     case TargetLowering::TypeSoftenFloat: {
       // Expand the floating point operand only if it was converted to integers.
       // Otherwise, it is a legal type like f128 that can be saved in a register.
       auto SoftenedOp = GetSoftenedFloat(InOp);
-      if (SoftenedOp == InOp)
+      if (isLegalInHWReg(SoftenedOp.getValueType()))
         break;
       SplitInteger(SoftenedOp, Lo, Hi);
       Lo = DAG.getNode(ISD::BITCAST, dl, NOutVT, Lo);
       Hi = DAG.getNode(ISD::BITCAST, dl, NOutVT, Hi);
       return;
     }
     case TargetLowering::TypeExpandInteger:
     case TargetLowering::TypeExpandFloat: {

llvm/trunk/test/CodeGen/X86/soft-fp-legal-in-HW-reg.ll

+; RUN: llc < %s -mtriple=x86_64-linux-android -mattr=+mmx -enable-legalize-types-checking | FileCheck %s
+;
+; D31946
+; Check that we dont end up with the ""LLVM ERROR: Cannot select" error.
+; Additionally ensure that the output code actually put fp128 values in SSE registers.
+
+declare fp128 @llvm.fabs.f128(fp128)
+declare fp128 @llvm.copysign.f128(fp128, fp128)
+
+define fp128 @TestSelect(fp128 %a, fp128 %b) {
+  %cmp = fcmp ogt fp128 %a, %b
+  %sub = fsub fp128 %a, %b
+  %res = select i1 %cmp, fp128 %sub, fp128 0xL00000000000000000000000000000000
+  ret fp128 %res
+; CHECK-LABEL: TestSelect:
+; CHECK        movaps 16(%rsp), %xmm1
+; CHECK-NEXT   callq __subtf3
+; CHECK-NEXT   testl %ebx, %ebx
+; CHECK-NEXT   jg .LBB0_2
+; CHECK-NEXT # BB#1:
+; CHECK-NEXT   movaps .LCPI0_0(%rip), %xmm0
+; CHECK-NEXT .LBB0_2:
+; CHECK-NEXT   addq $32, %rsp
+; CHECK-NEXT   popq %rbx
+; CHECK-NEXT   retq
+}
+
+define fp128 @TestFabs(fp128 %a) {
+  %res = call fp128 @llvm.fabs.f128(fp128 %a)
+  ret fp128 %res
+; CHECK-LABEL: TestFabs:
+; CHECK      andps .LCPI1_0(%rip), %xmm0
+; CHECK-NEXT retq
+}
+
+define fp128 @TestCopysign(fp128 %a, fp128 %b) {
+  %res = call fp128 @llvm.copysign.f128(fp128 %a, fp128 %b)
+  ret fp128 %res
+; CHECK-LABEL: TestCopysign:
+; CHECK      andps .LCPI2_1(%rip), %xmm0
+; CHECK-NEXT orps %xmm1, %xmm0
+; CHECK-NEXT retq
+}
+
+define fp128 @TestFneg(fp128 %a) {
+  %mul = fmul fp128 %a, %a
+  %res = fsub fp128 0xL00000000000000008000000000000000, %mul
+  ret fp128 %res
+; CHECK-LABEL: TestFneg:
+; CHECK      movaps %xmm0, %xmm1
+; CHECK-NEXT callq __multf3
+; CHECK-NEXT xorps .LCPI3_0(%rip), %xmm0
+; CHECK-NEXT popq %rax
+; CHECK-NEXT retq
+}