[gem5-dev] Change in gem5/gem5[develop]: arch-mips: Fix a bug in the MIPS yield instruction.

2021-03-05 Thread Gabe Black (Gerrit) via gem5-dev
Gabe Black has submitted this change. (  
https://gem5-review.googlesource.com/c/public/gem5/+/40957 )


Change subject: arch-mips: Fix a bug in the MIPS yield instruction.
..

arch-mips: Fix a bug in the MIPS yield instruction.

The yieldThread function implements MIPS's yield instruction, and had a
if condition in it, (src_reg && !yield_mask != 0), which upset clang. When
originally committed, this check read (src_reg & !yield_mask != 0), but
apparently as part of a cleanup sweep a long time ago, it was assumed
that the & was being used as a logical operator and was turned into &&.

Reading the actual description of what the yield instruction is supposed
to do, if src_reg is positive (it is at this point in the function),
then it's supposed to be treated as a bitvector. The YQMask register,
what gets passed in as yield_mask, can have bits set in it which mask
bits that might be set in src_reg, and if any are still set, the an
interrupt should happen, as implemented by the body of the if.

From this description, it's apparent that what the original code was
*trying* to do was to use yield_mask to mask any set bits in src_reg,
and then if any bits were left go into the body. The original author
used ! as a bitwise negating operator since what they *wanted* to do was
to block any bits in src_reg where yield_mask *is* set, and let through
any where yield_mask *is not* set. The & would do that, but only with a
bitwise negated yield_mask. Hence:

if ((src_reg & ~yield_mask) != 0) {
...
}

Change-Id: I30d0a47992750adf78c8aa0c28217da187e0cbda
Reviewed-on: https://gem5-review.googlesource.com/c/public/gem5/+/40957
Maintainer: Gabe Black 
Tested-by: kokoro 
Reviewed-by: Boris Shingarov 
---
M src/arch/mips/mt.hh
1 file changed, 1 insertion(+), 1 deletion(-)

Approvals:
  Boris Shingarov: Looks good to me, approved
  Gabe Black: Looks good to me, approved
  kokoro: Regressions pass



diff --git a/src/arch/mips/mt.hh b/src/arch/mips/mt.hh
index 9ab3290..da4c51a 100644
--- a/src/arch/mips/mt.hh
+++ b/src/arch/mips/mt.hh
@@ -273,7 +273,7 @@
 curTick(), tc->threadId());
 }
 } else if (src_reg > 0) {
-if (src_reg && !yield_mask != 0) {
+if ((src_reg & ~yield_mask) != 0) {
 VPEControlReg vpeControl =  
tc->readMiscReg(MISCREG_VPE_CONTROL);

 vpeControl.excpt = 2;
 tc->setMiscReg(MISCREG_VPE_CONTROL, vpeControl);



11 is the latest approved patch-set.
No files were changed between the latest approved patch-set and the  
submitted one.

--
To view, visit https://gem5-review.googlesource.com/c/public/gem5/+/40957
To unsubscribe, or for help writing mail filters, visit  
https://gem5-review.googlesource.com/settings


Gerrit-Project: public/gem5
Gerrit-Branch: develop
Gerrit-Change-Id: I30d0a47992750adf78c8aa0c28217da187e0cbda
Gerrit-Change-Number: 40957
Gerrit-PatchSet: 13
Gerrit-Owner: Gabe Black 
Gerrit-Reviewer: Andreas Sandberg 
Gerrit-Reviewer: Bobby R. Bruce 
Gerrit-Reviewer: Boris Shingarov 
Gerrit-Reviewer: Gabe Black 
Gerrit-Reviewer: Giacomo Travaglini 
Gerrit-Reviewer: kokoro 
Gerrit-MessageType: merged
___
gem5-dev mailing list -- gem5-dev@gem5.org
To unsubscribe send an email to gem5-dev-le...@gem5.org
%(web_page_url)slistinfo%(cgiext)s/%(_internal_name)s

[gem5-dev] Change in gem5/gem5[develop]: arch-mips: Fix a bug in the MIPS yield instruction.

2021-02-08 Thread Gabe Black (Gerrit) via gem5-dev
Gabe Black has uploaded this change for review. (  
https://gem5-review.googlesource.com/c/public/gem5/+/40957 )



Change subject: arch-mips: Fix a bug in the MIPS yield instruction.
..

arch-mips: Fix a bug in the MIPS yield instruction.

The yieldThread function implements MIPS's yield instruction, and had a
if condition in it, (src_reg && !yield_mask != 0), which upset clang. When
originally committed, this check read (src_reg & !yield_mask != 0), but
apparently as part of a cleanup sweep a long time ago, it was assumed
that the & was being used as a logical operator and was turned into &&.

Reading the actual description of what the yield instruction is supposed
to do, if src_reg is positive (it is at this point in the function),
then it's supposed to be treated as a bitvector. The YQMask register,
what gets passed in as yield_mask, can have bits set in it which mask
bits that might be set in src_reg, and if any are still set, the an
interrupt should happen, as implemented by the body of the if.

From this description, it's apparent that what the original code was
*trying* to do was to use yield_mask to mask any set bits in src_reg,
and then if any bits were left go into the body. The original author
used ! as a bitwise negating operator since what they *wanted* to do was
to block any bits in src_reg where yield_mask *is* set, and let through
any where yield_mask *is not* set. The & would do that, but only with a
bitwise negated yield_mask. Hence:

if ((src_reg & ~yield_mask) != 0) {
...
}

Change-Id: I30d0a47992750adf78c8aa0c28217da187e0cbda
---
M src/arch/mips/mt.hh
1 file changed, 1 insertion(+), 1 deletion(-)



diff --git a/src/arch/mips/mt.hh b/src/arch/mips/mt.hh
index 9ab3290..da4c51a 100644
--- a/src/arch/mips/mt.hh
+++ b/src/arch/mips/mt.hh
@@ -273,7 +273,7 @@
 curTick(), tc->threadId());
 }
 } else if (src_reg > 0) {
-if (src_reg && !yield_mask != 0) {
+if ((src_reg & ~yield_mask) != 0) {
 VPEControlReg vpeControl =  
tc->readMiscReg(MISCREG_VPE_CONTROL);

 vpeControl.excpt = 2;
 tc->setMiscReg(MISCREG_VPE_CONTROL, vpeControl);

--
To view, visit https://gem5-review.googlesource.com/c/public/gem5/+/40957
To unsubscribe, or for help writing mail filters, visit  
https://gem5-review.googlesource.com/settings


Gerrit-Project: public/gem5
Gerrit-Branch: develop
Gerrit-Change-Id: I30d0a47992750adf78c8aa0c28217da187e0cbda
Gerrit-Change-Number: 40957
Gerrit-PatchSet: 1
Gerrit-Owner: Gabe Black 
Gerrit-MessageType: newchange
___
gem5-dev mailing list -- gem5-dev@gem5.org
To unsubscribe send an email to gem5-dev-le...@gem5.org
%(web_page_url)slistinfo%(cgiext)s/%(_internal_name)s