[PATCH] D29265: [legalize-types] Remove stale entries from SoftenedFloats.

[Serge Pavlov via Phabricator via llvm-commits]

sepavloff added a comment.

In https://reviews.llvm.org/D29265#663586, @chh wrote:

> Could we have more info in this change or https://reviews.llvm.org/D29180?
>  Which recent change broke which tests? Could it be reverted?
>  What were the before and after regression IL trees?
>  Do we need new unit test?


There is no failing test or apparent problems in the legalizer. The intent is to run builds with expensive checks enabled on regular basis. To do this we need reach clean state, when all tests pass.

The real problem in this that expensive checks have been turned off for a long time and algorithm of checking is out of sync with current implementation of the legalizer. Both this change and https://reviews.llvm.org/D29180 try to fix the check algorithm so that it won't report problems were they absent.

> Last time I fixed a regression related to fp128 in https://reviews.llvm.org/D26942.
>  The regression was introduced by a cleanup with nice intention,
>  but we just do not have enough unit tests. We only found
>  the regression too late to revert, when we tried to compile
>  a large Android code base. Yes, fp128 could work for all other
>  targets except Android's x86-64. The SoftenFloat* functions got into
>  infinite loop. So I would be very careful now about removing nodes
>  from SoftenedFloats.

Absolutely agree. I think we should change only the check algorithm because the legalizer itself works as needed, at least we don't see any evidence that something is wrong with it.

The particular problem solved by this patch is that the check algorithm does not allow a replacement node be in both `SoftenedFloats` and `ReplacedValues`. I think it is wrong requirement. Essentially all the maps `SoftenedFloats`, `ReplacedValues` and other  are in fact the same thing, a map from a node to its replacement. Nodes from `ReplacedValues` are allowed to be overridden, why those from `SoftenedFloats` are not? Probably the legalizer could be implemented in such way that the aforementioned maps remain disjoint, but current implementation breaks this condition.


https://reviews.llvm.org/D29265