[PATCH] D31946: [legalize-types] Make softening result use a single map for replacements.

[Chih-Hung Hsieh via Phabricator via llvm-commits]

chh added a comment.

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 https://reviews.llvm.org/D29265.

https://reviews.llvm.org/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 https://reviews.llvm.org/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".


https://reviews.llvm.org/D31946