Differential D29265
[legalize-types] Remove stale entries from SoftenedFloats. Authored by fhahn on Jan 29 2017, 2:53 PM.
Details When replacing a SDValue, we should remove the replaced value from When we revisit a Node because it needs analyzing again, we have to This fixes the fp128 test failures with expensive checks for X86. I think we probably should also remove the values from the other maps
Diff Detail Event TimelineComment Actions D29180 tries to fix the same issues, by updating DAGTypeLegalizer::PerformExpensiveChecks to accept values in ReplacedValues and SoftenedFloats. I still think it would be better to properly clean up the stale entries. Comment Actions Just a discussion about approach, as we both worked on the same problem :) It looks like all these arrays, PromotedIntegers, SoftededFloats etc are in fact parts of one array that contains mapping of nodes to their replacement, they differ only by operation that created the replacement. They are useful only for error checking, otherwise their separate existence is useless. If it is true, we could have only one array, ReplacedValues that contained all such replacements. For the sake of checking we could keep few bits along with the replacement (in the form of PointerIntPair for instance) and check them when the replacement node is extracted or updated. Such checks would be cheap, they do not require search in tables, and they could be enabled in regular debug builds. I have feeling that implementation of the legalizer is overcomplicated, and if we get rid of separate arrays for replacement nodes, the legalizer will become more clear and, may be, faster. What do you think about such solution? Comment Actions In principle it sounds like a good idea to refactor the code and it indeed seems like the checks could be simplified. The main problem I think is that most of the code in DAGTypeLegalizer has not been touched for a very long time so it may be quite hard to find a reviewer. I think it would be ideal to merge this fix first and merge a bigger refactor later. Comment Actions I don't quite understand what's going on here. Can we visit a node, decide it needs to be a softened float, and then visit it again and make a different decision? Can't we already have used the result from the map? Comment Actions For the X86 fp128 tests, the following is going on as far as I understand: There is a node t9: f128 = select t8, t2, t4. During legalization of this node the following happens
But after thinking about it a little more I feel like there should be a better solution than my patch. Comment Actions +chh for comments on fp128 legalization. I think a common ReplacedValues map that also tracks the kind of legalization performed would be useful - at the very least to understand how the different legalization steps interact and let us code in sanity checks. Comment Actions Could we have more info in this change or D29180? Last time I fixed a regression related to fp128 in D26942. Comment Actions 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 D29180 try to fix the check algorithm so that it won't report problems were they absent.
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. Comment Actions Could you modify existing fp128 unit tests, especially those for Android target, to run with both current configuration and the "expensive checks". How do you enable the "expensive checks"? I remember dealing with some differences between SoftenedFloats and ReplacedValues maps, when I twisted various configurations to get fp128 values not softened for some operators and saved in mmx registers. I am afraid that a lot of cases were not tested with the "expensive checks" enabled. Comment Actions To enable all expensive checks is through a build define: LLVM_ENABLE_EXPENSIVE_CHECKS, to perform it on tests you have to enable the specific check in question, in this case "-enable-legalize-types-checking" Really we need build bot coverage that has LLVM_ENABLE_EXPENSIVE_CHECKS defined by default, but that can't happen until we have fixed all the current failures. Comment Actions In D29180, I saw that fp16.ll was changed to run with -enable-legalize-types-checking. Could you add extra test runs with the -enable-legalize-types-checking flag, into current fp128 tests and some other tests depending on SoftenedFloats? Then some of those runs will pass only if this change to LegalizeTypes.cpp is applied, right? Although I am not sure about the impact of this change to all SoftenedFloats targets, at least I have not seen regression in my local simple Android x86-64 build test. I hope someone else more familiar with this module can review this change. Thanks. Comment Actions Thank you for all the comments and sorry for my late response, I was away for the last 4 weeks. I've added -enable-legalize-types-checking to guard against the problem even when building without expensive checks. I've also done a quick git bisect and it seems like the failure was introduced in D11438, but after I think it looks more like the failure surfaced because the patch enabled softening for fp128 types. |