Unexpected behavior introduced in v1.08

[tq-z]

Hi,

I noticed the v1.08 unrolls the comparison loops manually here:

tn93/src/tn93_shared.cc

Lines 584 to 605 in 55565df

	for (; p + 2 <= span_start; p+=2) {
	unsigned c1 = s1[p], c2 = s2[p];
	unsigned c3 = s1[p+1], c4 = s2[p+1];
	if (__builtin_expect(c1 < 4 && c2 < 4, 1)) {
	integer_counts [c1][c2] ++;
	} else { // not both resolved
	if (c1 == GAP || c2 == GAP) {
	continue;
	}
	ambiguityHandler (c1,c2);
	}
	if (__builtin_expect(c3 < 4 && c4 < 4, 1)) {
	integer_counts2 [c3][c4] ++;
	} else { // not both resolved
	if (c3 == GAP || c4 == GAP) {
	continue;
	}
	ambiguityHandler (c3,c4);
	}
	}

. The continue at line 592 may make the comparison at line 597-604 skipped. Is it a new feature or bug?

Thanks,
Tianqi

[spond]

Dear @tq-z,

Well-spotted. This is a bug. I'll fix ASAP and release a new version (@stevenweaver -- please update our dependancies).

Best,
Sergei

[niemasd]

Thank you for fixing this so quickly, @spond and @stevenweaver!

[spond]

Dear @niemasd,

My pleasure. I am glad @tq-z found the issue. The manual unrolling was actually to allow some degree of CPU parallelism in this "chokepoint" loop with unavoidable data dependencies. In my hands, achieves 20-40% speedups.

Best,
Sergei

[niemasd]

Wow, 20-40% speedup is exciting! I look forward to trying it out!

Thanks,
-Niema