This patch originated from D46562 and is a proper subset, with some issues addressed.
Details
Diff Detail
Event Timeline
There's a lot going on here - in particular, the transforms guarded by 'reassoc'. I'm not sure those are correct as-is.
I know this is tedious, but I have to ask again to split this up and add tests, so we can see diffs for each fold using node-level FMF.
lib/CodeGen/SelectionDAG/DAGCombiner.cpp | ||
---|---|---|
707–709 | clang-format? | |
766–767 | Change FIXME: the FMF version is good now; the global check is wrong. (Might want to comment on the broken global checks in multiple places as an NFC preliminary step.) |
There are several transforms under the last part of this patch, and I'm not sure if we can do them without 'nsz'. This is very similar to D47335 (ping @wristow in case his memory of these cases is better than mine). In that patch, we decided to require 'nsz' for a bunch of reassociations rather than risk breaking some case that we weren't seeing. Should we do that here too at least as a first step? I'm cursing 'nsz' because I just botched that one in D48085.
Here's an example:
(fadd (fadd x, c1), c2) -> (fadd x, (fadd c1, c2))
define float @fadd_consts(float %x) { %a1 = fadd float %x, 4.200000e+01 %a2 = fadd reassoc float %a1, 1.700000e+01 ret float %a2 }
We don't fold that in IR (instcombine) currently unless that 2nd fadd has 'nsz' too.
Added a todo comment and added nsz to the constraint for the collection of transformations marked.
lib/CodeGen/SelectionDAG/DAGCombiner.cpp | ||
---|---|---|
10351 | I think I will change "false" to "considered" |
lib/CodeGen/SelectionDAG/DAGCombiner.cpp | ||
---|---|---|
10338–10339 | This seems broken independently of this patch - we just said it's not safe to create new constants, and every transform under here creates a new constant. But we only check 'AllowNewConst' for some of the folds? Also, I updated the 1st transform under here to not check hasOneUse() in rL334608, so you'll need to rebase. |
Sync'd up to head of tree, adjusted one test (newly added here), some minor refactoring for 'AllowNewConst' and add the cleanup from SelectionDAG since fmul, fsub and fadd are all updated to handle what getNode was doing (that code is removed).
lib/CodeGen/SelectionDAG/DAGCombiner.cpp | ||
---|---|---|
10379–10384 | This transformation and the similar ones below can change the NaN-ness of the result. For example, consider x = +inf, c = -0.5. In that case, the LHS is NaN (because of the intermediate -inf + +inf), while the RHS is +inf. Is reassoc supposed to be a strong enough condition to allow this? I don't see a way for a non-NaN result to be transformed into a NaN result, so perhaps this is okay? But this needs to be a conscious decision and it should be mentioned in the comment above that talks about rounding steps. |
lib/CodeGen/SelectionDAG/DAGCombiner.cpp | ||
---|---|---|
10379–10384 | This is circular reasoning, but LLVM does this fold already in IR as long as we have 'nsz reassoc'. This is also allowed by gcc with the equivalent flags: Our 'reassoc' definition currently says: We could add to that definition if we can make that clearer? Or as you suggest, we can document the potential differences more clearly here by including the INF example. |
lib/CodeGen/SelectionDAG/DAGCombiner.cpp | ||
---|---|---|
10379–10384 | Those are good points. It did catch my eye because the documentation of reassoc led me down the path of thinking that sure, there's going to be differences based on different rounding. But this particular difference isn't caused by rounding differences. Having slept on it and with your points, I think it's okay to do this transform based on reassoc because it doesn't create new NaNs as far as I can tell. If there were similar cases where new NaNs can be created, I think we should tread more carefully. (Of course "mere" rounding differences can create new NaNs, e.g. by turning inf * tiny number into inf * 0, but those should be expected by floating point practicioners, which makes it okay.) |
lib/CodeGen/SelectionDAG/DAGCombiner.cpp | ||
---|---|---|
10338–10339 | Shouldn't the 'AllowNewConst' check be moved up here rather than repeated with each transform below? Again, I think that particular change is independent of the FMF change, so it should be reviewed/committed as a separate patch (preferably before we make the FMF change, so we're not increasing the chance of hitting the bug). Not sure how/if we can make that difference visible in a regression test with in-tree targets. Remove the existing variable in trunk and see if anything breaks? |
lib/CodeGen/SelectionDAG/DAGCombiner.cpp | ||
---|---|---|
10338–10339 | The AllowNewConst change is NFC, is I will track it as a simple check in, then update this review afterwards. |
lib/CodeGen/SelectionDAG/DAGCombiner.cpp | ||
---|---|---|
10338–10339 | I wound up keeping some of the refactoring so it is not quite NFC, the review should appear shortly. The resultant change here in this review will make the code change quite small. |
LGTM - see inline for a nit.
lib/CodeGen/SelectionDAG/DAGCombiner.cpp | ||
---|---|---|
10360–10362 | I'd prefer that we not perpetuate UnsafeFPMath any more than necessary, so I'd say something like: |
lib/CodeGen/SelectionDAG/DAGCombiner.cpp | ||
---|---|---|
10360–10362 | I am going to remove the comment, if we leave a special message about reassocation, someone can infer it is different from Unsafe and therefore new behavior, when it is not. |
clang-format?