[gem5-dev] Change in gem5/gem5[develop]: arch-gcn3: fix bits that SDWA selects
Anthony Gutierrez has submitted this change. (
https://gem5-review.googlesource.com/c/public/gem5/+/29931 )
Change subject: arch-gcn3: fix bits that SDWA selects
..
arch-gcn3: fix bits that SDWA selects
This commit fixes a bug in 200f2408 where the SDWA support was selecting
bits
backwards. As part of this commit, to help resolve this problem in the
future, I have added asserts in the helper functions in bitfield.hh to
ensure
that the number of bits aren't negative.
Change-Id: I4b0ecb0e7c110600c0b5063101b75f9adcc512ac
Reviewed-on: https://gem5-review.googlesource.com/c/public/gem5/+/29931
Tested-by: kokoro
Reviewed-by: Anthony Gutierrez
Reviewed-by: Matt Sinclair
Maintainer: Anthony Gutierrez
---
M src/arch/gcn3/insts/inst_util.hh
M src/base/bitfield.hh
2 files changed, 37 insertions(+), 30 deletions(-)
Approvals:
Anthony Gutierrez: Looks good to me, approved; Looks good to me, approved
Matt Sinclair: Looks good to me, but someone else must approve
kokoro: Regressions pass
diff --git a/src/arch/gcn3/insts/inst_util.hh
b/src/arch/gcn3/insts/inst_util.hh
index 292e3ba..433ccbe 100644
--- a/src/arch/gcn3/insts/inst_util.hh
+++ b/src/arch/gcn3/insts/inst_util.hh
@@ -551,7 +551,7 @@
const SDWASelVals sel, const bool signExt)
{
// local variables
-int first_bit = 0, last_bit = 0;
+int low_bit = 0, high_bit = 0;
bool signExt_local = signExt;
T retVal = 0;
@@ -566,17 +566,19 @@
of byte 0, or makes the bits of the selected byte be byte 0
(and
next either sign extends or zero's out upper bits).
*/
-first_bit = (sel * Gcn3ISA::BITS_PER_BYTE);
-last_bit = first_bit + Gcn3ISA::MSB_PER_BYTE;
-retVal = bits(currOperVal, first_bit, last_bit);
+low_bit = (sel * Gcn3ISA::BITS_PER_BYTE);
+high_bit = low_bit + Gcn3ISA::MSB_PER_BYTE;
+retVal = bits(currOperVal, high_bit, low_bit);
// make sure update propagated, since used next
-assert(bits(retVal, Gcn3ISA::MSB_PER_BYTE) ==
- bits(origOperVal, (sel * Gcn3ISA::BITS_PER_BYTE) +
-Gcn3ISA::MSB_PER_BYTE));
+fatal_if(bits(retVal, Gcn3ISA::MSB_PER_BYTE) !=
+ bits(origOperVal, high_bit),
+ "ERROR: SDWA byte update not propagated: retVal: %d, "
+ "orig: %d\n", bits(retVal, Gcn3ISA::MSB_PER_BYTE),
+ bits(origOperVal, high_bit));
// sign extended value depends on upper-most bit of the new
byte 0
signExt_local = (signExt &&
- (bits(retVal, 0, Gcn3ISA::MSB_PER_BYTE) &
0x80));
+ (bits(retVal, Gcn3ISA::MSB_PER_BYTE, 0) &
0x80));
// process all other bytes -- if sign extending, make them 1,
else
// all 0's so leave as is
@@ -589,17 +591,20 @@
of word 0, or makes the bits of the selected word be word 0
(and
next either sign extends or zero's out upper bits).
*/
-first_bit = (sel & 1) * Gcn3ISA::BITS_PER_WORD;
-last_bit = first_bit + Gcn3ISA::MSB_PER_WORD;
-retVal = bits(currOperVal, first_bit, last_bit);
+low_bit = (sel & 1) * Gcn3ISA::BITS_PER_WORD;
+high_bit = low_bit + Gcn3ISA::MSB_PER_WORD;
+retVal = bits(currOperVal, high_bit, low_bit);
// make sure update propagated, since used next
-assert(bits(retVal, Gcn3ISA::MSB_PER_WORD) ==
- bits(origOperVal, ((sel & 1) * Gcn3ISA::BITS_PER_WORD) +
-Gcn3ISA::MSB_PER_WORD));
+fatal_if(bits(retVal, Gcn3ISA::MSB_PER_WORD) !=
+ bits(origOperVal, high_bit),
+ "ERROR: SDWA word update not propagated: retVal: %d, "
+ "orig: %d\n",
+ bits(retVal, Gcn3ISA::MSB_PER_WORD),
+ bits(origOperVal, high_bit));
// sign extended value depends on upper-most bit of the new
word 0
signExt_local = (signExt &&
- (bits(retVal, 0, Gcn3ISA::MSB_PER_WORD) &
+ (bits(retVal, Gcn3ISA::MSB_PER_WORD, 0) &
0x8000));
// process other word -- if sign extending, make them 1, else
all
@@ -659,7 +664,7 @@
const SDWADstVals unusedBits_format)
{
// local variables
-int first_bit = 0, last_bit = 0;
+int low_bit = 0, high_bit = 0;
bool signExt = (unusedBits_format == SDWA_UNUSED_SEXT);
//bool pad = (unusedBits_format == SDWA_UNUSED_PAD);
bool preserve = (unusedBits_format == SDWA_UNUSED_PRESERVE);
@@ -679,11 +684,11
[gem5-dev] Change in gem5/gem5[develop]: arch-gcn3: fix bits that SDWA selects
Hello Tony Gutierrez,
I'd like you to do a code review. Please visit
https://gem5-review.googlesource.com/c/public/gem5/+/29931
to review the following change.
Change subject: arch-gcn3: fix bits that SDWA selects
..
arch-gcn3: fix bits that SDWA selects
This commit fixes a bug in 200f2408 where the SDWA support was selecting
bits
backwards. As part of this commit, to help resolve this problem in the
future, I have added asserts in the helper functions in bitfield.hh to
ensure
that the number of bits aren't negative.
Change-Id: I4b0ecb0e7c110600c0b5063101b75f9adcc512ac
---
M src/arch/gcn3/insts/inst_util.hh
M src/base/bitfield.hh
2 files changed, 37 insertions(+), 30 deletions(-)
diff --git a/src/arch/gcn3/insts/inst_util.hh
b/src/arch/gcn3/insts/inst_util.hh
index 292e3ba..433ccbe 100644
--- a/src/arch/gcn3/insts/inst_util.hh
+++ b/src/arch/gcn3/insts/inst_util.hh
@@ -551,7 +551,7 @@
const SDWASelVals sel, const bool signExt)
{
// local variables
-int first_bit = 0, last_bit = 0;
+int low_bit = 0, high_bit = 0;
bool signExt_local = signExt;
T retVal = 0;
@@ -566,17 +566,19 @@
of byte 0, or makes the bits of the selected byte be byte 0
(and
next either sign extends or zero's out upper bits).
*/
-first_bit = (sel * Gcn3ISA::BITS_PER_BYTE);
-last_bit = first_bit + Gcn3ISA::MSB_PER_BYTE;
-retVal = bits(currOperVal, first_bit, last_bit);
+low_bit = (sel * Gcn3ISA::BITS_PER_BYTE);
+high_bit = low_bit + Gcn3ISA::MSB_PER_BYTE;
+retVal = bits(currOperVal, high_bit, low_bit);
// make sure update propagated, since used next
-assert(bits(retVal, Gcn3ISA::MSB_PER_BYTE) ==
- bits(origOperVal, (sel * Gcn3ISA::BITS_PER_BYTE) +
-Gcn3ISA::MSB_PER_BYTE));
+fatal_if(bits(retVal, Gcn3ISA::MSB_PER_BYTE) !=
+ bits(origOperVal, high_bit),
+ "ERROR: SDWA byte update not propagated: retVal: %d, "
+ "orig: %d\n", bits(retVal, Gcn3ISA::MSB_PER_BYTE),
+ bits(origOperVal, high_bit));
// sign extended value depends on upper-most bit of the new
byte 0
signExt_local = (signExt &&
- (bits(retVal, 0, Gcn3ISA::MSB_PER_BYTE) &
0x80));
+ (bits(retVal, Gcn3ISA::MSB_PER_BYTE, 0) &
0x80));
// process all other bytes -- if sign extending, make them 1,
else
// all 0's so leave as is
@@ -589,17 +591,20 @@
of word 0, or makes the bits of the selected word be word 0
(and
next either sign extends or zero's out upper bits).
*/
-first_bit = (sel & 1) * Gcn3ISA::BITS_PER_WORD;
-last_bit = first_bit + Gcn3ISA::MSB_PER_WORD;
-retVal = bits(currOperVal, first_bit, last_bit);
+low_bit = (sel & 1) * Gcn3ISA::BITS_PER_WORD;
+high_bit = low_bit + Gcn3ISA::MSB_PER_WORD;
+retVal = bits(currOperVal, high_bit, low_bit);
// make sure update propagated, since used next
-assert(bits(retVal, Gcn3ISA::MSB_PER_WORD) ==
- bits(origOperVal, ((sel & 1) * Gcn3ISA::BITS_PER_WORD) +
-Gcn3ISA::MSB_PER_WORD));
+fatal_if(bits(retVal, Gcn3ISA::MSB_PER_WORD) !=
+ bits(origOperVal, high_bit),
+ "ERROR: SDWA word update not propagated: retVal: %d, "
+ "orig: %d\n",
+ bits(retVal, Gcn3ISA::MSB_PER_WORD),
+ bits(origOperVal, high_bit));
// sign extended value depends on upper-most bit of the new
word 0
signExt_local = (signExt &&
- (bits(retVal, 0, Gcn3ISA::MSB_PER_WORD) &
+ (bits(retVal, Gcn3ISA::MSB_PER_WORD, 0) &
0x8000));
// process other word -- if sign extending, make them 1, else
all
@@ -659,7 +664,7 @@
const SDWADstVals unusedBits_format)
{
// local variables
-int first_bit = 0, last_bit = 0;
+int low_bit = 0, high_bit = 0;
bool signExt = (unusedBits_format == SDWA_UNUSED_SEXT);
//bool pad = (unusedBits_format == SDWA_UNUSED_PAD);
bool preserve = (unusedBits_format == SDWA_UNUSED_PRESERVE);
@@ -679,11 +684,11 @@
if (sel < SDWA_WORD_0) { // we are selecting 1 byte
// if we sign extended depends on upper-most bit of byte 0
signExt = (signExt &&
- (bits(currDstVal, 0, Gcn3ISA::MSB_PER_WORD) &
0x80));
+ (bits(currDstVal,
