fteqw icon indicating copy to clipboard operation
fteqw copied to clipboard

Optimizations of FP conditional creates inconsistent results

Open 4LT opened this issue 1 year ago • 6 comments

I was reviewing the Ironwail source and noticed that floating point values are coerced to integer by type punning in conditionals

	case OP_IF:
		if (OPA->_int)
			st += st->b - 1;	/* -1 to offset the st++ */
		break;

I expect the code is like this in the original Quake source? Anyways, I thought I'd try out the behavior of -0.0 since the floating point representation in binary isn't all zeroes. However, surprisingly, running the following code

if (-0.0) {
  dprint("truthy\n");
} else {
  dprint("falsy\n");
}

produces the result of printing "falsy". I suspect this might be due to -0.0 being interpreted as a float == to 0.0 in the compiler and getting optimized out. Sure enough, when I tried the following code, the game prints "truthy" instead:

float x = 0.0/-1.0;
if (x) {
  dprint("truthy\n");
} else {
  dprint("falsy\n");
}

4LT avatar Oct 04 '23 22:10 4LT

Looks like the issue comes in here: https://github.com/fte-team/fteqw/blob/eb6b127d9ce10949f00bcad8c5953b7092c92d12/engine/qclib/qcc_pr_comp.c#L13934-L13953

4LT avatar Oct 04 '23 22:10 4LT

That is, according to @Shpoike, vanilla accurate behaviour. It can be disabled by compiling your progs with -fiffloat

https://github.com/fte-team/fteqw/blob/eb6b127d9ce10949f00bcad8c5953b7092c92d12/engine/qclib/qccmain.c#L399

eukara avatar Oct 05 '23 17:10 eukara

I understand that -0.0 being truthy is consistent with vanilla engine behavior, my concern is that

if (-0.0) {

is behaviorally different from

float x = 0.0/-1.0;
if (x) {

unless that's also consistent with vanilla QCC?

4LT avatar Oct 05 '23 17:10 4LT

I understand that -0.0 being truthy is consistent with vanilla engine behavior, my concern is that

if (-0.0) {

is behaviorally different from

float x = 0.0/-1.0;
if (x) {

unless that's also consistent with vanilla QCC?

sorry, I'm on the assumption that the discrepancy is due to optimization and that the result of 0.0/-1.0 is exactly -0.0; I'll check these assumptions when I get a chance

4LT avatar Oct 05 '23 18:10 4LT

I have confirmed my assumptions: replacing 0.0/-1.0 with -0.0 in the assignment to x has no effect.

To summarize, the following code

    float x = -0.0;

    if (-0.0) {
        dprint("truthy\n");
    } else {
        dprint("falsy\n");
    }

    if (x) {
        dprint("truthy\n");
    } else {
        dprint("falsy\n");
    }

prints the following in console:

falsy
truthy

4LT avatar Oct 09 '23 22:10 4LT

if (e.cast->type == ev_float && flag_iffloat) striptruth = ...

that should 'fix' it... for some definition of 'fix'... (might be better to warn about -0 tests) Of course, the logic will then fail in DP afterwards, you can't win em all.

Frankly I'm a bit more concerned that it doesn't seem to evaluate other types properly either, like vectors(with flag_ifvector), longs, doubles, or strings(with flag_ifstring). There is a QCC_Eval_Truth function which would be more appropriate, but it follows and/or rules so assumes flag_iffloat is set. for, (do)while and (do)until may have similar issues. it should be noted that flag_iffloat is normally effectively forced on whenever fteqcc has the OP_IF(NOT)_F opcode, because the whole -0 thing is obscene - the flag was originally to specify that you're okay with spitting out a whole load of extra opcodes for every if statement (fteqcc is meant to have hidden booleans now so if(a&&b) won't cost extra because it knows the temp stores something that can only be 0 or 1 and not -0, but if(a) sadly will when its a random variable and sadly even if its set on the prior line, which is why its not defaulted because qc has a lot of such tests).

Shpoike avatar Oct 15 '23 00:10 Shpoike