As per discution on how to get better codegen an large int legalization, it became clear that using a glue for the carry was preventing several desirable optimizations. Passing the carry down as a value allow for more flexibility.
Details
Diff Detail
- Build Status
Buildable 4877 Build 4877: arc lint + arc unit
Event Timeline
Haven't looked in great detail, but overall looks sane to me.
It'll be great to also eliminate ADDC/ADDE/SUBC/SUBE, instead of adding an alternative method. Oh...and, SETCCE. Will probably need a new variant of that as well?
The FIXME for x86 seems pretty distasteful...
Possibly could handle things by having ADDCARRY A, B, CARRY -> RES, OUTCARRY expand into something along the lines of:
- Create FLAGS value per CARRY input. (what's best way to spell that? New pseudo-op "setcarry" that expands into "ADD8RI CARRY, 255 -> unused, FLAGS", maybe?)
- Do addition, using carry frmo flags register, input and output: x86isd::ADC A, B, FLAGS -> RES, OUTFLAGS
- Grab carry bit from flags register: x86isd::setcc cond_b, OUTFLAGS -> OUTCARRY
Then, have a transform that converts "(setcarry (setcc cond_b FLAGS)) -> FLAGS" into just passing through FLAGS?
include/llvm/CodeGen/ISDOpcodes.h | ||
---|---|---|
232 | Value should be allowed to come from anywhere, not just uaddo/addcarry, although those would be most common. |
It'll be great to also eliminate ADDC/ADDE/SUBC/SUBE, instead of adding an alternative method. Oh...and, SETCCE. Will probably need a new variant of that as well?
Yes I think that where we want to go with this, but proceeding incrementally seems to make more sense. My plan was to get the new stuff up, move in tree backend to use it, and then deprecate it/remove it. SETCCE is also on my radar.
The FIXME for x86 seems pretty distasteful...
Yes I was very unhappy with that solution and was hopping for some advice on how to do this better. I'll try your solution and see how it goes. Do you see the setcarry as a generic ISD or as a X86 specific one ?
Fixed all test regressions. Turned out adding proper support for addcarry and uaddo in computeKnownBits was the missing piece.
If possible it'd be great to split this down into smaller patches, which might be quicker to review overall.
lib/CodeGen/SelectionDAG/DAGCombiner.cpp | ||
---|---|---|
2048 | Split this into a separate small patch + tests? | |
lib/CodeGen/SelectionDAG/SelectionDAG.cpp | ||
2548 | Could the ISD::UADDO case be split into a separate small patch + tests? | |
lib/Target/X86/X86ISelLowering.cpp | ||
34343 | Again, can this be extracted into its own patch + tests? |
lib/CodeGen/SelectionDAG/DAGCombiner.cpp | ||
---|---|---|
2048 | No you get a bunch of regression without these. This is doing the same transforms as for ADDC, so this isn't actually new. | |
lib/CodeGen/SelectionDAG/SelectionDAG.cpp | ||
2548 | Same thing, bunch of regression without this. | |
lib/Target/X86/X86ISelLowering.cpp | ||
34343 | Without this, you get an add with a carry, followed by a setb or an sbb instruction to materialise the carry, followed by an add Carry, -1 to regenerate the carry. It's pretty bad. |
What I meant was to pull out some of the codegen changes into smaller patches with their own tests that can be reviewed and accepted before this patch.
Then make this patch just about the addition of ADDCARRY and SUBCARRY instead of having to handle so many things at once.
I see, I can do that, but I'm afraid the test cases will be poor. In this patch the tests cases that exists for ADC are covering the transform, but these tests case can't be leveraged when not using UADDO/ADDCARRY .
include/llvm/CodeGen/ISDOpcodes.h | ||
---|---|---|
236 | Please explicitly note that the carry input to SUBCARRY supposed to be the output of USUBO; the "carry" bit is defined differently on some architectures. |
Remove all UADDO/USUBO related changes in their own diff.
Keep only the ADDCARRY/SUBCARRY related code in there.
lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp | ||
---|---|---|
516–521 | The can only be 2 or 3 element so i think just allocating for 2 on the stack is preferable. This is done in various other places in the codebase already. | |
lib/Target/X86/X86ISelLowering.cpp | ||
23288 | Because in that case, the backend wants to run some extra legalization code before coming back there. |
A few minors for you to look at
include/llvm/CodeGen/ISDOpcodes.h | ||
---|---|---|
235 | Please detail the results as well (similar to ADDE/SUBE). | |
lib/CodeGen/SelectionDAG/DAGCombiner.cpp | ||
8256–8257 | Update comment to describe addcarry too. | |
lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp | ||
517 | This will be clearer with unsigned instead of auto | |
1281 | Mixed use of SDValue and auto. | |
1768 | clang-format | |
1925 | clang-format |
lib/Target/X86/X86ISelLowering.cpp | ||
---|---|---|
23288 | At the time this code runs, there should not be any illegal types present in the DAG. |
lib/Target/X86/X86ISelLowering.cpp | ||
---|---|---|
23288 | I'm not sure what the X86 backend is doing, but this is clearly happening looking at the -debug output of llc. |
lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp | ||
---|---|---|
516–521 | Add explicit SDValue() to initialize Ops[2] ? | |
1812 | Should this be a separate fix? | |
1914 | Should this be added separately? At very least DAGTypeLegalizer::ExpandIntRes_UADDSUBO should be moved as an NFC pre-commit so that diff is more obvious. | |
lib/Target/X86/X86ISelLowering.cpp | ||
34318 | We already have a combineAdd function - this should be named something like combineX86Add |
I haven't looked at carry codegen enough to approve, but see inline comments for some nits.
include/llvm/CodeGen/ISDOpcodes.h | ||
---|---|---|
215–218 | If I'd just stumbled into this code, I'd be confused why we have ADDC and ADDE and ADDCARRY. If the intent is to remove these, please add a 'FIXME' comment here that says we should get rid of these. | |
lib/CodeGen/SelectionDAG/DAGCombiner.cpp | ||
2077–2079 | This is copied from above, but comments are supposed to be full sentences (capital letter, period). | |
lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp | ||
517 | I'd call this 'NumOps' or similar. 'C' in LLVM usually means some kind of constant value. | |
lib/CodeGen/TargetLoweringBase.cpp | ||
913–914 | This code comment won't age well. Delete. | |
lib/Target/X86/X86ISelLowering.cpp | ||
34318 | +1 - we definitely do not want function names that only differ in captitalization. | |
34324 | auto -> SDValue | |
34336 | auto -> auto * |
lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp | ||
---|---|---|
1948 | iff is a shorthand for if and only if, to indicate bijection. it is commonly used in the comment int hat file, so I think this is fine. |
Ping ?
I'd really would ike to move on with optimizing big integers better and this is gating everything. This needs to move forward.
include/llvm/CodeGen/ISDOpcodes.h | ||
---|---|---|
215–218 | I agree with Sanjay - you need to document the plan for ADDC/SUBC here | |
lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp | ||
1921 | HasOpCarry (style) | |
1934 | Please unsigned instead of auto for the simple types | |
1967 | SDValue HiOps[3] = { LHSH, RHSH, SDValue() }; | |
lib/Target/X86/X86ISelLowering.cpp | ||
23285 | unsigned | |
23303 | SDValue |
Add a FIXME on ADDC/SUBC to explain they are deprecated in favor of ADDCARRY and SUBCARRY and will be removed in the future.
Replaced several uses of auto with explicit types as per request.
I see nothing to stop this being accepted now - does anyone have any further comments?
Having the FIXME note was my main concern...just in case the transition takes longer than expected. I have no other comments.
If you guys are good with it, then let's move forward. I have various patches coming based on this, and we start moving backends to use ADDCARRY instead of ADC once it is in.
If I'd just stumbled into this code, I'd be confused why we have ADDC and ADDE and ADDCARRY. If the intent is to remove these, please add a 'FIXME' comment here that says we should get rid of these.