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

[Anton Yartsev via Phabricator via llvm-commits]

ayartsev added a comment.

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

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

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.


https://reviews.llvm.org/D31946