[gem5-dev] Change in gem5/gem5[develop]: arch-gcn3, gpu-compute: Fix issue when reading const operands

[Anthony Gutierrez (Gerrit) via gem5-dev]

Anthony Gutierrez has submitted this change. (  
https://gem5-review.googlesource.com/c/public/gem5/+/29927 )


Change subject: arch-gcn3, gpu-compute: Fix issue when reading const  
operands

..

arch-gcn3, gpu-compute: Fix issue when reading const operands

Currently, when an instruction has an operand that reads a const
value, it goes thru the same readMiscReg() api call as other
misc registers (real HW registers, not constant values). There
is an issue, however, when casting from the const values (which are
32b) to higher precision values, like 64b.

This change creates a separate, templated function call to the GPU's
ISA state that will return the correct type.

Change-Id: I41965ebeeed20bb70e919fce5ad94d957b3af802
Reviewed-on: https://gem5-review.googlesource.com/c/public/gem5/+/29927
Reviewed-by: Anthony Gutierrez 
Maintainer: Anthony Gutierrez 
Tested-by: kokoro 
---
M src/arch/gcn3/gpu_isa.hh
M src/arch/gcn3/isa.cc
M src/arch/gcn3/operand.hh
M src/arch/gcn3/registers.cc
M src/arch/gcn3/registers.hh
M src/gpu-compute/gpu_exec_context.hh
6 files changed, 66 insertions(+), 17 deletions(-)

Approvals:
  Anthony Gutierrez: Looks good to me, approved; Looks good to me, approved
  kokoro: Regressions pass



diff --git a/src/arch/gcn3/gpu_isa.hh b/src/arch/gcn3/gpu_isa.hh
index 26b79c7..228c3fe 100644
--- a/src/arch/gcn3/gpu_isa.hh
+++ b/src/arch/gcn3/gpu_isa.hh
@@ -37,6 +37,7 @@
 #define __ARCH_GCN3_GPU_ISA_HH__

 #include 
+#include 

 #include "arch/gcn3/registers.hh"
 #include "gpu-compute/dispatcher.hh"
@@ -52,6 +53,24 @@
   public:
 GPUISA(Wavefront );

+template T
+readConstVal(int opIdx) const
+{
+panic_if(!std::is_integral::value, "Constant values must "
+ "be an integer.\n");
+T val(0);
+
+if (isPosConstVal(opIdx)) {
+val = (T)readPosConstReg(opIdx);
+}
+
+if (isNegConstVal(opIdx)) {
+val = (T)readNegConstReg(opIdx);
+}
+
+return val;
+}
+
 ScalarRegU32 readMiscReg(int opIdx) const;
 void writeMiscReg(int opIdx, ScalarRegU32 operandVal);
 bool hasScalarUnit() const { return true; }
@@ -63,10 +82,9 @@
 return posConstRegs[opIdx - REG_INT_CONST_POS_MIN];
 }

-ScalarRegU32 readNegConstReg(int opIdx) const
+ScalarRegI32 readNegConstReg(int opIdx) const
 {
-return *((ScalarRegU32*)
-[opIdx - REG_INT_CONST_NEG_MIN]);
+return negConstRegs[opIdx - REG_INT_CONST_NEG_MIN];
 }

 static const std::array
diff --git a/src/arch/gcn3/isa.cc b/src/arch/gcn3/isa.cc
index 036c771..3bd122d 100644
--- a/src/arch/gcn3/isa.cc
+++ b/src/arch/gcn3/isa.cc
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2016-2017 Advanced Micro Devices, Inc.
+ * Copyright (c) 2016-2018 Advanced Micro Devices, Inc.
  * All rights reserved.
  *
  * For use for simulation and test purposes only
@@ -49,14 +49,6 @@
 ScalarRegU32
 GPUISA::readMiscReg(int opIdx) const
 {
-if (opIdx >= REG_INT_CONST_POS_MIN && opIdx <=  
REG_INT_CONST_POS_MAX) {

-return readPosConstReg(opIdx);
-}
-
-if (opIdx >= REG_INT_CONST_NEG_MIN && opIdx <=  
REG_INT_CONST_NEG_MAX) {

-return readNegConstReg(opIdx);
-}
-
 switch (opIdx) {
   case REG_M0:
 return m0;
diff --git a/src/arch/gcn3/operand.hh b/src/arch/gcn3/operand.hh
index 218faf8..7f70fab 100644
--- a/src/arch/gcn3/operand.hh
+++ b/src/arch/gcn3/operand.hh
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2017 Advanced Micro Devices, Inc.
+ * Copyright (c) 2017-2018 Advanced Micro Devices, Inc.
  * All rights reserved.
  *
  * For use for simulation and test purposes only
@@ -583,10 +583,15 @@
   default:
 {
 assert(sizeof(DataType) <= sizeof(srfData));
-DataType misc_val
-= (DataType)_gpuDynInst->readMiscReg(_opIdx);
+DataType misc_val(0);
+if (isConstVal(_opIdx)) {
+misc_val = (DataType)_gpuDynInst
+->readConstVal(_opIdx);
+} else {
+misc_val =  
(DataType)_gpuDynInst->readMiscReg(_opIdx);

+}
 std::memcpy((void*)srfData.data(), (void*)_val,
-sizeof(DataType));
+sizeof(DataType));
 }
 }
 }
diff --git a/src/arch/gcn3/registers.cc b/src/arch/gcn3/registers.cc
index 0872ff9..016160f 100644
--- a/src/arch/gcn3/registers.cc
+++ b/src/arch/gcn3/registers.cc
@@ -163,6 +163,31 @@
 }

 bool
+isPosConstVal(int opIdx)
+{
+bool is_pos_const_val = (opIdx >= REG_INT_CONST_POS_MIN
+&& opIdx <= 

[gem5-dev] Change in gem5/gem5[develop]: arch-gcn3, gpu-compute: Fix issue when reading const operands

[Anthony Gutierrez (Gerrit) via gem5-dev]

Hello Tony Gutierrez,

I'd like you to do a code review. Please visit

https://gem5-review.googlesource.com/c/public/gem5/+/29927

to review the following change.


Change subject: arch-gcn3, gpu-compute: Fix issue when reading const  
operands

..

arch-gcn3, gpu-compute: Fix issue when reading const operands

Currently, when an instruction has an operand that reads a const
value, it goes thru the same readMiscReg() api call as other
misc registers (real HW registers, not constant values). There
is an issue, however, when casting from the const values (which are
32b) to higher precision values, like 64b.

This change creates a separate, templated function call to the GPU's
ISA state that will return the correct type.

Change-Id: I41965ebeeed20bb70e919fce5ad94d957b3af802
---
M src/arch/gcn3/gpu_isa.hh
M src/arch/gcn3/isa.cc
M src/arch/gcn3/operand.hh
M src/arch/gcn3/registers.cc
M src/arch/gcn3/registers.hh
M src/gpu-compute/gpu_exec_context.hh
6 files changed, 66 insertions(+), 17 deletions(-)



diff --git a/src/arch/gcn3/gpu_isa.hh b/src/arch/gcn3/gpu_isa.hh
index 26b79c7..228c3fe 100644
--- a/src/arch/gcn3/gpu_isa.hh
+++ b/src/arch/gcn3/gpu_isa.hh
@@ -37,6 +37,7 @@
 #define __ARCH_GCN3_GPU_ISA_HH__

 #include 
+#include 

 #include "arch/gcn3/registers.hh"
 #include "gpu-compute/dispatcher.hh"
@@ -52,6 +53,24 @@
   public:
 GPUISA(Wavefront );

+template T
+readConstVal(int opIdx) const
+{
+panic_if(!std::is_integral::value, "Constant values must "
+ "be an integer.\n");
+T val(0);
+
+if (isPosConstVal(opIdx)) {
+val = (T)readPosConstReg(opIdx);
+}
+
+if (isNegConstVal(opIdx)) {
+val = (T)readNegConstReg(opIdx);
+}
+
+return val;
+}
+
 ScalarRegU32 readMiscReg(int opIdx) const;
 void writeMiscReg(int opIdx, ScalarRegU32 operandVal);
 bool hasScalarUnit() const { return true; }
@@ -63,10 +82,9 @@
 return posConstRegs[opIdx - REG_INT_CONST_POS_MIN];
 }

-ScalarRegU32 readNegConstReg(int opIdx) const
+ScalarRegI32 readNegConstReg(int opIdx) const
 {
-return *((ScalarRegU32*)
-[opIdx - REG_INT_CONST_NEG_MIN]);
+return negConstRegs[opIdx - REG_INT_CONST_NEG_MIN];
 }

 static const std::array
diff --git a/src/arch/gcn3/isa.cc b/src/arch/gcn3/isa.cc
index 036c771..3bd122d 100644
--- a/src/arch/gcn3/isa.cc
+++ b/src/arch/gcn3/isa.cc
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2016-2017 Advanced Micro Devices, Inc.
+ * Copyright (c) 2016-2018 Advanced Micro Devices, Inc.
  * All rights reserved.
  *
  * For use for simulation and test purposes only
@@ -49,14 +49,6 @@
 ScalarRegU32
 GPUISA::readMiscReg(int opIdx) const
 {
-if (opIdx >= REG_INT_CONST_POS_MIN && opIdx <=  
REG_INT_CONST_POS_MAX) {

-return readPosConstReg(opIdx);
-}
-
-if (opIdx >= REG_INT_CONST_NEG_MIN && opIdx <=  
REG_INT_CONST_NEG_MAX) {

-return readNegConstReg(opIdx);
-}
-
 switch (opIdx) {
   case REG_M0:
 return m0;
diff --git a/src/arch/gcn3/operand.hh b/src/arch/gcn3/operand.hh
index 218faf8..7f70fab 100644
--- a/src/arch/gcn3/operand.hh
+++ b/src/arch/gcn3/operand.hh
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2017 Advanced Micro Devices, Inc.
+ * Copyright (c) 2017-2018 Advanced Micro Devices, Inc.
  * All rights reserved.
  *
  * For use for simulation and test purposes only
@@ -583,10 +583,15 @@
   default:
 {
 assert(sizeof(DataType) <= sizeof(srfData));
-DataType misc_val
-= (DataType)_gpuDynInst->readMiscReg(_opIdx);
+DataType misc_val(0);
+if (isConstVal(_opIdx)) {
+misc_val = (DataType)_gpuDynInst
+->readConstVal(_opIdx);
+} else {
+misc_val =  
(DataType)_gpuDynInst->readMiscReg(_opIdx);

+}
 std::memcpy((void*)srfData.data(), (void*)_val,
-sizeof(DataType));
+sizeof(DataType));
 }
 }
 }
diff --git a/src/arch/gcn3/registers.cc b/src/arch/gcn3/registers.cc
index 0872ff9..016160f 100644
--- a/src/arch/gcn3/registers.cc
+++ b/src/arch/gcn3/registers.cc
@@ -163,6 +163,31 @@
 }

 bool
+isPosConstVal(int opIdx)
+{
+bool is_pos_const_val = (opIdx >= REG_INT_CONST_POS_MIN
+&& opIdx <= REG_INT_CONST_POS_MAX);
+
+return is_pos_const_val;
+}
+
+bool
+isNegConstVal(int opIdx)
+{
+bool is_neg_const_val = (opIdx >= REG_INT_CONST_NEG_MIN
+&& opIdx <=