fold (addc X, (adde Y, 0, Carry)) -> (adde X, Y, Carry)
One step towards improving pr31719
Diff Detail
Diff Detail
- Repository
- rL LLVM
Event Timeline
| lib/CodeGen/SelectionDAG/DAGCombiner.cpp | ||
|---|---|---|
| 1872 | Is that important to check this has only one use ? I'd assume that having (adde Y, 0, Carry) + (adde X, Y, Carry) is still preferable to (addc X, (adde Y, 0, Carry)) as it break dependencies between instructions. | |
| lib/CodeGen/SelectionDAG/DAGCombiner.cpp | ||
|---|---|---|
| 1872 | That's a fair point, but here I am following the practice that is common in the DAG Combiner. If you search for "hasOneUse" or "isOnlyUserOf" you can see the conservative approach of avoiding increase in instruction count. | |
| lib/CodeGen/SelectionDAG/DAGCombiner.cpp | ||
|---|---|---|
| 1872 | Are there any situations where the inner ADDE's carry flag will be used by something else? | |
| lib/CodeGen/SelectionDAG/DAGCombiner.cpp | ||
|---|---|---|
| 1872 | The carry's type is glue, so by definition it can have a single user, right? | |
| lib/CodeGen/SelectionDAG/DAGCombiner.cpp | ||
|---|---|---|
| 1872 |
I don't think this is a valid concern in that case. If the instruction has more than one use, you get 2 instructions before and 2 after. So I don't think this apply in that case. However, I'm not sure about the glue. It may indeed be the case that it is expected that a glue is used only once (that would make sense). In which case maybe a adding comment is a good thing because if it is not obvious to us now, it won't be to some dude reading this in a few month without context. | |
| lib/CodeGen/SelectionDAG/DAGCombiner.cpp | ||
|---|---|---|
| 1872 | You can see for yourself the results of your suggestion by applying this patch and running 'make check-llvm'. | |
| lib/CodeGen/SelectionDAG/DAGCombiner.cpp | ||
|---|---|---|
| 1872 | Alright, I think this is moot anyway. I'm not sure this transformation is valid if Y may be all ones. adde would generate a carry, but the addc would not, after the transformation, it would generate a carry. | |
| lib/CodeGen/SelectionDAG/DAGCombiner.cpp | ||
|---|---|---|
| 1872 | I suggest something like: // (addc X, (adde Y, 0, Carry)) -> (adde X, Y, Carry) if Y + 1 cannot overflow.
if (N1.getOpcode() == ISD::ADDE &&
N->isOnlyUserOf(N1.getValue(0).getNode()) &&
isNullConstant(N1.getOperand(1))) {
APInt YZero, YOne;
DAG.computeKnownBits(N1.getOperand(0), YZero, YOne);
if (YZero)
return DAG.getNode(ISD::ADDE, SDLoc(N), N->getVTList(), N0,
N1->getOperand(0), N1->getOperand(2));
} | |
| lib/CodeGen/SelectionDAG/DAGCombiner.cpp | ||
|---|---|---|
| 1872 | For what it's worth, in the meantime, I did https://reviews.llvm.org/D29443 which solve the whole thing. This pattern is part of the solution. | |
Comment Actions
I don't mind abandoning this patch in favor of D29443, but if possible, i think you should break D29443 into several patches. I'm saying this because D29443 adds several new combines and it would be easier to review each combine in a separate patch with tests that show its goodness.
So it may turn out that after breaking D29443, this patch will end-up as the initial patch in a sequence. Does this make sense?
Comment Actions
I actually posted in that diff asking if I should do this, so yes, it does make sense.
Is that important to check this has only one use ? I'd assume that having (adde Y, 0, Carry) + (adde X, Y, Carry) is still preferable to (addc X, (adde Y, 0, Carry)) as it break dependencies between instructions.