-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
cranelift: Remove iadd_cin
instruction
#5166
Comments
What would be the purpose of removing iadd_cin while keeping iadd_carry? It is useful for terminating a bigint addition. |
I'm proposing removing However It outputs the carry if either of the two
|
iadd_cin is iadd_carry except that it doesn't materialize the carry flag. In other words iadd_cin can be implemented using a single adc instruction on x86 while iadd_carry must be implemented as adc+setc if not fused with the instruction using the output carry flag. |
Right, but in our case we have no guarantee that the previous input has set the carry flag unless we pattern match some previous instruction. So we need to pattern mach the carry input of |
What I'm proposing is replacing the following:
With this:
Edit: In the We actually can't do this exact pattern matching yet, since ISLE can't match two output instructions, but the same goes for |
Maybe the value is more clear here for wider integers? It seems like
would, in general, need to be
in order to not have (but it's also possible that I completely don't understand cranelift's type expectations) |
👋 Hey,
This operation is essentially two
add
's chained together, I think that any optimizations that we make based on this operation can also be made on theiadd
instruction, so there doesn't seem to be any benefit of keeping this around other than frontend convenience. And if that is important, maybe we should just move this to a frontend helper that expands to twoiadd
's + some masking for the carry input?Additionally since the removal of booleans, the input for the carry bit seems a bit under-specified. We need some form of masking on the carry input, either
carry & 1
orcarry != 0
. (This also affects theiadd_carry
instruction).We discussed carry operations last week in #5123 and also in the cranelift weekly meeting (notes) but nothing concrete about
iadd_cin
I think.Some previous discussion about this mentions that cg-clif may need
iadd_cin
to "efficiently implement certain llvm intrinsics used by core::arch intrinsics that are stabilized" and points to the current implementation but that seems to be implementingiadd_carry
which I agree probably needs to remain.@bjorn3 Do you know of any operation that cg-clif would need to do, that needs
iadd_cin
but couldn't efficiently be replaced withiadd+iadd
?For reference here's the llvm output for this instruction, and as far as I can tell, it doesn't seem to be doing anything particularly special for any of the arches that we support.
For the
carry & 1
it does emit theand
instruction + twoadd
's.And for the
carry != 0
it does do something special on aarch64 and x86, but we can also pattern matchiadd+icmp not zero
and emit that same code.The text was updated successfully, but these errors were encountered: