Skip to content
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

teach kAND/kOR to optimize &(1) and |(1) #976

Merged
merged 1 commit into from
Dec 8, 2023

Conversation

oleg-nesterov
Copy link
Contributor

These BinOp's can optimize out the isMinusOne/isZero operands, but since the xxxNeutral/Absorbing predicates take a single argument they can't check another operand and take isOne() into account, so this patch adds the brute force check into simplification.

Random example,

process = _ <: ==(1) | ==(2) <: &(1==1), |(2!=3);

compiles to

float fTemp0 = float(input0[i0]);
int iTemp1 = (fTemp0 == 1.0f) | (fTemp0 == 2.0f);
output0[i0] = FAUSTFLOAT(iTemp1 & 1);
output1[i0] = FAUSTFLOAT(iTemp1 | 1);

after this patch we have

float fTemp0 = float(input0[i0]);
output0[i0] = FAUSTFLOAT((fTemp0 == 1.0f) | (fTemp0 == 2.0f));
output1[i0] = FAUSTFLOAT(1);

Should the new isSigBool() function have the "int max_recursion" argument?

These BinOp's can optimize out the isMinusOne/isZero operands, but since
the xxxNeutral/Absorbing predicates take a single argument they can't check
another operand and take isOne() into account, so this patch adds the brute
force check into simplification.

Random example,

	process = _ <: ==(1) | ==(2) <: &(1==1), |(2!=3);

compiles to

	float fTemp0 = float(input0[i0]);
	int iTemp1 = (fTemp0 == 1.0f) | (fTemp0 == 2.0f);
	output0[i0] = FAUSTFLOAT(iTemp1 & 1);
	output1[i0] = FAUSTFLOAT(iTemp1 | 1);

after this patch we have

	float fTemp0 = float(input0[i0]);
	output0[i0] = FAUSTFLOAT((fTemp0 == 1.0f) | (fTemp0 == 2.0f));
	output1[i0] = FAUSTFLOAT(1);

Should the new isSigBool() function have the "int max_recursion" argument?
@sletz
Copy link
Member

sletz commented Nov 28, 2023

Thanks, should we also consider optimising&(0)and |(0) ?

@oleg-nesterov
Copy link
Contributor Author

Hi,

I can't check this right now, but I think this is not needed. I think that absorbing/neutral
predicates (can't recall the correct names right now) should already take care about this
case.

At least this is what I had in mind when I wrote this patch, and I even tried to document
this in the changelog, please see the note about isZero() operands at the start.

Thanks,

@oleg-nesterov
Copy link
Contributor Author

I'm afraid I wasn't clear, let me add more spam.

Lets suppose we have "expr & 0". In this case we don't care about "expr", result is 0
and iirc simplification() already does this using the absorbing (or neutral, I can confuse
the names ;)

But "expr & 1" is more difficult, we can remove "& 1" sometimes, but we need to inspect
this "expr". This is what the patch does, and this is what neutral/absorbing can't do because
they accept a single argument: 0 or 1 in these cases.

@sletz
Copy link
Member

sletz commented Nov 28, 2023

OK right, I will test ASAP, but possible not before next week

@sletz sletz merged commit e03c425 into grame-cncm:master-dev Dec 8, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants