Inplace binary operation class for the OpenFHE IR + sum and sub inplace operations

[ShirChoiEX]

This is my attempt of adding the inplace operations for the Openfhe IR. I added inplace addition/subtraction definitions to check if it would build, which it did.

I would like to have some comments on the implementation, if it is correct, and how can it be improved.

[google-cla]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

[ShirChoiEX]

This is my attempt of adding the inplace operations for the Openfhe IR. I added inplace addition/subtraction definitions to check if it would build, which it did.

I would like to have some comments on the implementation, if it is correct, and how can it be improved.

[ZenithalHourlyRate]

Thanks for the contribution!

Please install a pre-commit hook (check https://heir.dev/docs/contributing/) to make PR format clean.

Others are great!

[ZenithalHourlyRate]

There is a recent PR (#1334) that relaxes the AllTypesMatch requirement to [SameOperandsAndResultRings, InferTypeOpAdaptor]

Discussion for maintainers: We might also need to register a MemoryEffects<[MemWrite]> to make liveness analysis happy, though I found --cse --canonicalize does not eliminate these inplace ops.

[ShirChoiEX]

is there a need to add [SameOperandsAndResultRings] trait for the inplace operations? It seems to be analyzing both inputs and the output, but there is no output for an inplace function. There seem to be no alternative trait like [SameOperandsRings], or does the [SameOperandsAndResultRings] ignore the result check if it is not created? Also, regarding the #1334 PR, he uses [SameOperandsAndResultRings] in AddOp and SubOp but not in the MulOp why is it so?

[AlexanderViand-Intel]

Also, regarding the #1334 PR, he uses [SameOperandsAndResultRings] in AddOp and SubOp but not in the MulOp why is it so?

If the question is why no SameOperandsAndResultRings is needed for openfhe.mul it's because
AllTypesMatch<["lhs", "rhs", "output"]> implies SameOperandsAndResultRings, but if the question is instead why I didn't "relax" the constraints on openfhe.mul to "only" SameOperandsAndResultRings, that's just because the op isn't used in our default lowering pipeline so there was no need to.

EDIT: Note that the AllTypesMatch<["lhs", "rhs", "output"]> was already applied to openfhe.mul before my PR, it was just previously inherited from Openfhe_BinaryOp

[ZenithalHourlyRate]

or does the [SameOperandsAndResultRings] ignore the result check if it is not created?

From the following line we can know that if there is no result, the trait only checks the equivalence among operands.

heir/lib/Dialect/LWE/IR/LWETraits.h

Lines 19 to 50 in ad59e8f

	static LogicalResult verifyTrait(Operation *op) {
	::mlir::heir::polynomial::RingAttr rings = nullptr;
	for (auto rTy : op->getResultTypes()) {
	auto ct = dyn_cast<lwe::NewLWECiphertextType>(rTy);
	if (!ct) continue;
	if (rings == nullptr) {
	rings = ct.getCiphertextSpace().getRing();
	continue;
	}
	if (rings != ct.getCiphertextSpace().getRing()) {
	return op->emitOpError()
	<< "requires all operands and results to have the same rings";
	}
	}
	for (auto oTy : op->getOperandTypes()) {
	auto ct = dyn_cast<lwe::NewLWECiphertextType>(oTy);
	if (!ct) continue; // Check only ciphertexts
	if (rings == nullptr) {
	rings = ct.getCiphertextSpace().getRing();
	continue;
	}
	if (rings != ct.getCiphertextSpace().getRing()) {
	return op->emitOpError()
	<< "requires all operands and results to have the same rings";
	}
	}
	return success();
	}
	};

[ShirChoiEX]

I rebased the branch + fixed merge conflicts. All tests should be passing now

[j2kun]

Sorry for the last minute comment. Otherwise, LGTM!

[j2kun]

I'll get this in an patch the minor nitpick internally :)