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

[MIPS] Implement llvm.fminimum and llvm.fmaximum with f32/f64 #89907

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Cyanoxygen
Copy link
Contributor

@Cyanoxygen Cyanoxygen commented Apr 24, 2024

  • The implementation used various checks against signed zeroes and NaNs to ensure the correct value is returned.

Depends on the following PR:

Closes #64207

@wzssyqa
Copy link
Contributor

wzssyqa commented Apr 27, 2024

@Cyanoxygen the CI test fails.

@Cyanoxygen
Copy link
Contributor Author

@Cyanoxygen the CI test fails.

Let me examine the log.

@Cyanoxygen
Copy link
Contributor Author

./build/bin/llvm-lit --path ~/.local/lib/llvm llvm/test/CodeGen/Mips/mipsr6-minmaxnum.ll
-- Testing: 1 tests, 1 workers --
PASS: LLVM :: CodeGen/Mips/mipsr6-minmaxnum.ll (1 of 1)

Testing Time: 0.07s

Total Discovered Tests: 1
  Passed: 1 (100.00%)

Maybe the problem is on this merge b8ae81f. Rebasing the branch.

@Cyanoxygen
Copy link
Contributor Author

Cyanoxygen commented Apr 27, 2024

For mipsel-linux-gnu, it is neither soft float nor FP32, nor FP64, nor FPXX.
Updating code using Subtarget.hasSoftFloat.

The implementation used various checks against signed zeroes and NaNs to
ensure the correct value is returned.
@wzssyqa
Copy link
Contributor

wzssyqa commented Apr 29, 2024

I think that this logic may introduce better performance:

if (a>b) {
	return a;
}
if (a<b) {
	return b;
}
if (a == b) {
	if (a == -0)  // with GPR instead of FPR.
		return b;
	return a;
}
if (a unordered 0) {
	return a;
} else {
	return b;
}

Can you have consider it?
It's due to that we process the most cases as early as possible.

@Cyanoxygen
Copy link
Contributor Author

Cyanoxygen commented Apr 29, 2024

Can you have consider it?
It's due to that we process the most cases as early as possible.

preliminary checks are essential since we do not know what value actually is in the operands, and since minnum and maxnum returns other operand if NaN exists, we have to check if there's any NaN in them.

I have a “slightly better” version in mind, which might handle the edge case. Again, preliminary checks are essential:

newx = max(x, y)
if (newx == x) {
	if (y == NaN) {
		return y;
	} else {
		if (y == 0) {
			if (x == -0) {
				return y;
			} else {
				return x;
			}
		}  else {
			return x;
		}
	}
} else {
	return y;
}

Which roughly translates to:

(SELECT
	(SETCC NewX X SETUEQ)
	(SELECT
		(SETCC Y Y SETUO) ; Y == NaN then return Y
		Y
		(SELECT
			(SETCC Y 0 SETEQ)
			(SELECT
				(SETCC X NegZero)
				Y X)
			X)
		X)
	)
	Y)

@wzssyqa
Copy link
Contributor

wzssyqa commented Apr 29, 2024

Can you have consider it?
It's due to that we process the most cases as early as possible.

preliminary checks are essential since we do not know what value actually is in the operands, and since minnum and maxnum returns other operand if NaN exists, we have to check if there's any NaN in them.

I have a “slightly better” version in mind, which might handle the edge case. Again, preliminary checks are essential:

newx = max(x, y)
if (newx == x) {
	if (y == NaN) {
		return y;
	}
	if (y == 0) {
		if (x == -0) return y;
		return x;
	}
	return x;
}
return y;

Maybe it is better for R6, but not for pre-R6.
In fact for R6, we can do it better

newx = max(x, y)
if ((x is NaN) or (y is NaN)) {
     return NaN;
}
return newx;

max.fmt in R6 can process +0/-0 as fmaximum expects.

@Cyanoxygen
Copy link
Contributor Author

max.fmt in R6 can process +0/-0 as fmaximum expects.

Oh yes you are right. min/max.fmt actually considers +0 > -0.

@Cyanoxygen
Copy link
Contributor Author

Converting back to draft, while I working on the logic based on the discussion above.

@Cyanoxygen Cyanoxygen marked this pull request as draft April 29, 2024 06:39
@wzssyqa
Copy link
Contributor

wzssyqa commented Apr 29, 2024

I figure out a pre-R6 asm code

xm:
	.frame	$sp,0,$31		# vars= 0, regs= 0/0, args= 0, gp= 0
	.mask	0x00000000,0
	.fmask	0x00000000,0
	.set	noreorder
	.set	nomacro
	c.lt.d	$fcc0,$f14,$f12
	bc1t	$fcc0,$L6
	mov.d	$f0,$f12

	c.lt.d	$fcc0,$f12,$f14
	bc1t	$fcc0,$L6
	mov.d	$f0,$f14

	c.eq.d $fcc0,$f14,$f12
	bc1f	$fcc0,$L4
	mfhc1	$t4,$f14
	bltz	$t4,$L6
	mov.d	$f0,$f12

	mfhc1	$t4,$f12
	bltz	$t4,$L6
	mov.d	$f0,$f14

$L4:
	mtc1	$0,$f4
	mthc1	$0,$f4
	c.un.d	$fcc0,$f12,$f4
	bc1t	$fcc0,$L6
	mov.d	$f0,$f12

	# we have $f14 already
$L6:
	jr	$31
	nop

I will feed back it to glibc.

@Cyanoxygen
Copy link
Contributor Author

Can you have consider it?
It's due to that we process the most cases as early as possible.

preliminary checks are essential since we do not know what value actually is in the operands, and since minnum and maxnum returns other operand if NaN exists, we have to check if there's any NaN in them.

I have a “slightly better” version in mind, which might handle the edge case. Again, preliminary checks are essential:

newx = max(x, y)
if (newx == x) {
	if (y == NaN) {
		return y;
	} else {
		if (y == 0) {
			if (x == -0) {
				return y;
			} else {
				return x;
			}
		}  else {
			return x;
		}
	}
} else {
	return y;
}

Which roughly translates to:

(SELECT
	(SETCC NewX X SETUEQ)
	(SELECT
		(SETCC Y Y SETUO) ; Y == NaN then return Y
		Y
		(SELECT
			(SETCC Y 0 SETEQ)
			(SELECT
				(SETCC X NegZero)
				Y X)
			X)
		X)
	)
	Y)

I just discovered that the logic above has a loophole. If X is a NaN, and Y is a real number, then the real number will be returned.

As this gets way complicated to implement, I would like to keep the original implementation. But we can handle this specifically for R6.

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.

[MIPS] Cannot select llvm.{min,max}imum.{f32,f64}
2 participants