Hi Nilay, The first two issues pointed out by Giacomo are not addressed.
May 22nd: In general, I like the idea of adding a vector type, as it's going to be a more future-proof solution, and ARMv8 code could benefit as well from that... but I have a few concerns regarding this code as it stands currently: 1. The patch doesn't address the problem of overlapping registers (for now it's only xmm's & ymm's, but zmm's are likely to join the party soon); in this context, it's not clear to me if we'd need something fundamentally different already at this stage to address that - the risk is to upstream this code and then later discover that we need a different strategy for overlapping. Maybe just having a plan on how to address that in the future could be enough at this stage? 2. In the interim we would have broken SSE-AVX/AVX-2 interworking. Now here I'm not sure where the gem5 community draws the line in terms of ISA correctness vs. getting code out of the door for early experiments, etc. :) At least it would be good to have some thoughts from the wider gem5 community here... We would really appreciate if this can be reverted so that we can resolve the aforementioned issues before any changes are pushed. Thanks, Andreas On 27/07/2015 15:51, "gem5-dev on behalf of Nilay Vaish" <[email protected] on behalf of [email protected]> wrote: >Andreas, can you point out what issue has not been addressed in my >responses? In my opinion everything has been. > >-- >Nilay > >On Mon, 27 Jul 2015, Andreas Hansson wrote: > >> Hi Nilay, >> >> Can we please revert this changeset and leave the review open? As >>already >> stated in the review comments, there are some critical issues that lead >>us >> to believe that this patch does not bring us closer to a solution. >> >> It would be much appreciated if we can keep this code in review until >>all >> these issues are resolved and we have established a solution that works >> more broadly. >> >> Thanks, >> >> Andreas >> >> On 26/07/2015 16:21, "gem5-dev on behalf of Nilay Vaish" >> <[email protected] on behalf of [email protected]> wrote: >> >>> changeset 5af8f40d8f2c in /z/repo/gem5 >>> details: http://repo.gem5.org/gem5?cmd=changeset;node=5af8f40d8f2c >>> description: >>> cpu: implements vector registers >>> >>> This adds a vector register type. The type is defined as a >>>std::array >>> of a >>> fixed number of uint64_ts. The isa_parser.py has been modified >>>to parse >>> vector >>> register operands and generate the required code. Different >>>cpus have >>> vector >>> register files now. >>> >>> diffstat: >>> >>> src/arch/SConscript | 4 +- >>> src/arch/alpha/isa.hh | 7 ++ >>> src/arch/alpha/registers.hh | 10 ++- >>> src/arch/alpha/utility.cc | 1 + >>> src/arch/arm/insts/static_inst.cc | 2 + >>> src/arch/arm/isa.hh | 7 ++ >>> src/arch/arm/registers.hh | 10 ++- >>> src/arch/arm/utility.cc | 3 + >>> src/arch/isa_parser.py | 115 >>> +++++++++++++++++++++++++++++++++++- >>> src/arch/mips/isa.hh | 7 ++ >>> src/arch/mips/registers.hh | 10 ++- >>> src/arch/mips/utility.cc | 3 + >>> src/arch/null/registers.hh | 2 + >>> src/arch/power/insts/static_inst.cc | 2 + >>> src/arch/power/isa.hh | 7 ++ >>> src/arch/power/registers.hh | 10 ++- >>> src/arch/power/utility.cc | 3 + >>> src/arch/sparc/isa.hh | 7 ++ >>> src/arch/sparc/registers.hh | 9 ++- >>> src/arch/sparc/utility.cc | 3 + >>> src/arch/x86/insts/static_inst.cc | 7 ++ >>> src/arch/x86/isa.hh | 6 + >>> src/arch/x86/registers.hh | 11 +++- >>> src/arch/x86/utility.cc | 4 + >>> src/cpu/StaticInstFlags.py | 7 +- >>> src/cpu/base_dyn_inst.hh | 26 ++++++++ >>> src/cpu/checker/cpu.hh | 24 +++++++- >>> src/cpu/checker/cpu_impl.hh | 74 ++++++++++++++++------ >>> src/cpu/checker/thread_context.hh | 16 +++++ >>> src/cpu/exec_context.hh | 17 +++++ >>> src/cpu/minor/dyn_inst.cc | 2 + >>> src/cpu/minor/exec_context.hh | 43 +++++++++---- >>> src/cpu/minor/scoreboard.cc | 8 ++ >>> src/cpu/minor/scoreboard.hh | 12 ++- >>> src/cpu/o3/O3CPU.py | 7 ++ >>> src/cpu/o3/cpu.cc | 76 +++++++++++++++++++++++- >>> src/cpu/o3/cpu.hh | 12 +++ >>> src/cpu/o3/dyn_inst.hh | 19 +++++- >>> src/cpu/o3/free_list.hh | 21 +++++- >>> src/cpu/o3/inst_queue_impl.hh | 2 +- >>> src/cpu/o3/regfile.cc | 26 ++++++- >>> src/cpu/o3/regfile.hh | 52 +++++++++++++++- >>> src/cpu/o3/rename_impl.hh | 20 +++++- >>> src/cpu/o3/rename_map.cc | 12 +++ >>> src/cpu/o3/rename_map.hh | 41 ++++++++++++- >>> src/cpu/o3/thread_context.hh | 12 +++ >>> src/cpu/o3/thread_context_impl.hh | 23 +++++++ >>> src/cpu/reg_class.cc | 1 + >>> src/cpu/reg_class.hh | 6 +- >>> src/cpu/simple/base.hh | 20 ++++++ >>> src/cpu/simple_thread.hh | 50 +++++++++++++++ >>> src/cpu/static_inst.hh | 12 ++- >>> src/cpu/thread_context.cc | 30 +++++++++ >>> src/cpu/thread_context.hh | 24 +++++++ >>> src/sim/insttracer.hh | 9 ++- >>> 55 files changed, 876 insertions(+), 78 deletions(-) >>> >>> diffs (truncated from 2354 to 300 lines): >>> >>> diff -r e1309937d313 -r 5af8f40d8f2c src/arch/SConscript >>> --- a/src/arch/SConscript Sun Jul 26 10:20:07 2015 -0500 >>> +++ b/src/arch/SConscript Sun Jul 26 10:21:20 2015 -0500 >>> @@ -196,5 +196,7 @@ >>> DebugFlag('IntRegs') >>> DebugFlag('FloatRegs') >>> DebugFlag('CCRegs') >>> +DebugFlag('VectorRegs') >>> DebugFlag('MiscRegs') >>> -CompoundFlag('Registers', [ 'IntRegs', 'FloatRegs', 'CCRegs', >>>'MiscRegs' >>> ]) >>> +CompoundFlag('Registers', [ 'IntRegs', 'FloatRegs', 'CCRegs', >>> 'VectorRegs', >>> + 'MiscRegs' ]) >>> diff -r e1309937d313 -r 5af8f40d8f2c src/arch/alpha/isa.hh >>> --- a/src/arch/alpha/isa.hh Sun Jul 26 10:20:07 2015 -0500 >>> +++ b/src/arch/alpha/isa.hh Sun Jul 26 10:21:20 2015 -0500 >>> @@ -114,6 +114,13 @@ >>> return reg; >>> } >>> >>> + // dummy >>> + int >>> + flattenVectorIndex(int reg) const >>> + { >>> + return reg; >>> + } >>> + >>> int >>> flattenMiscIndex(int reg) const >>> { >>> diff -r e1309937d313 -r 5af8f40d8f2c src/arch/alpha/registers.hh >>> --- a/src/arch/alpha/registers.hh Sun Jul 26 10:20:07 2015 -0500 >>> +++ b/src/arch/alpha/registers.hh Sun Jul 26 10:21:20 2015 -0500 >>> @@ -56,6 +56,12 @@ >>> // dummy typedef since we don't have CC regs >>> typedef uint8_t CCReg; >>> >>> +// vector register file entry type >>> +typedef uint64_t VectorRegElement; >>> +const int NumVectorRegElements = 0; >>> +const int VectorRegBytes = NumVectorRegElements * >>> sizeof(VectorRegElement); >>> +typedef std::array<VectorRegElement, NumVectorRegElements> VectorReg; >>> + >>> union AnyReg >>> { >>> IntReg intreg; >>> @@ -95,6 +101,7 @@ >>> const int NumIntRegs = NumIntArchRegs + NumPALShadowRegs; >>> const int NumFloatRegs = NumFloatArchRegs; >>> const int NumCCRegs = 0; >>> +const int NumVectorRegs = 0; >>> const int NumMiscRegs = NUM_MISCREGS; >>> >>> const int TotalNumRegs = >>> @@ -106,7 +113,8 @@ >>> // 32..63 are the FP regs 0..31, i.e. use (reg + FP_Reg_Base) >>> FP_Reg_Base = NumIntRegs, >>> CC_Reg_Base = FP_Reg_Base + NumFloatRegs, >>> - Misc_Reg_Base = CC_Reg_Base + NumCCRegs, // NumCCRegs == 0 >>> + Vector_Reg_Base = CC_Reg_Base + NumCCRegs, // NumCCRegs == 0 >>> + Misc_Reg_Base = Vector_Reg_Base + NumCCRegs, // NumVectorRegs == 0 >>> Max_Reg_Index = Misc_Reg_Base + NumMiscRegs + NumInternalProcRegs >>> }; >>> >>> diff -r e1309937d313 -r 5af8f40d8f2c src/arch/alpha/utility.cc >>> --- a/src/arch/alpha/utility.cc Sun Jul 26 10:20:07 2015 -0500 >>> +++ b/src/arch/alpha/utility.cc Sun Jul 26 10:21:20 2015 -0500 >>> @@ -73,6 +73,7 @@ >>> >>> // Would need to add condition-code regs if implemented >>> assert(NumCCRegs == 0); >>> + assert(NumVectorRegs == 0); >>> >>> // Copy misc. registers >>> copyMiscRegs(src, dest); >>> diff -r e1309937d313 -r 5af8f40d8f2c src/arch/arm/insts/static_inst.cc >>> --- a/src/arch/arm/insts/static_inst.cc Sun Jul 26 10:20:07 >>>2015 -0500 >>> +++ b/src/arch/arm/insts/static_inst.cc Sun Jul 26 10:21:20 >>>2015 -0500 >>> @@ -337,6 +337,8 @@ >>> case CCRegClass: >>> ccprintf(os, "cc_%s", ArmISA::ccRegName[rel_reg]); >>> break; >>> + case VectorRegClass: >>> + panic("ARM ISA does not have any vector registers yet!"); >>> } >>> } >>> >>> diff -r e1309937d313 -r 5af8f40d8f2c src/arch/arm/isa.hh >>> --- a/src/arch/arm/isa.hh Sun Jul 26 10:20:07 2015 -0500 >>> +++ b/src/arch/arm/isa.hh Sun Jul 26 10:21:20 2015 -0500 >>> @@ -288,6 +288,13 @@ >>> } >>> >>> int >>> + flattenVectorIndex(int reg) const >>> + { >>> + assert(reg >= 0); >>> + return reg; >>> + } >>> + >>> + int >>> flattenMiscIndex(int reg) const >>> { >>> assert(reg >= 0); >>> diff -r e1309937d313 -r 5af8f40d8f2c src/arch/arm/registers.hh >>> --- a/src/arch/arm/registers.hh Sun Jul 26 10:20:07 2015 -0500 >>> +++ b/src/arch/arm/registers.hh Sun Jul 26 10:21:20 2015 -0500 >>> @@ -72,6 +72,12 @@ >>> // condition code register; must be at least 32 bits for FpCondCodes >>> typedef uint64_t CCReg; >>> >>> +// vector register file entry type >>> +typedef uint64_t VectorRegElement; >>> +const int NumVectorRegElements = 0; >>> +const int VectorRegBytes = NumVectorRegElements * >>> sizeof(VectorRegElement); >>> +typedef std::array<VectorRegElement, NumVectorRegElements> VectorReg; >>> + >>> // Constants Related to the number of registers >>> const int NumIntArchRegs = NUM_ARCH_INTREGS; >>> // The number of single precision floating point registers >>> @@ -82,6 +88,7 @@ >>> const int NumIntRegs = NUM_INTREGS; >>> const int NumFloatRegs = NumFloatV8ArchRegs + NumFloatSpecialRegs; >>> const int NumCCRegs = NUM_CCREGS; >>> +const int NumVectorRegs = 0; >>> const int NumMiscRegs = NUM_MISCREGS; >>> >>> #define ISA_HAS_CC_REGS >>> @@ -112,7 +119,8 @@ >>> // These help enumerate all the registers for dependence tracking. >>> const int FP_Reg_Base = NumIntRegs * (MODE_MAXMODE + 1); >>> const int CC_Reg_Base = FP_Reg_Base + NumFloatRegs; >>> -const int Misc_Reg_Base = CC_Reg_Base + NumCCRegs; >>> +const int Vector_Reg_Base = CC_Reg_Base + NumCCRegs; >>> +const int Misc_Reg_Base = Vector_Reg_Base + NumVectorRegs; >>> const int Max_Reg_Index = Misc_Reg_Base + NumMiscRegs; >>> >>> typedef union { >>> diff -r e1309937d313 -r 5af8f40d8f2c src/arch/arm/utility.cc >>> --- a/src/arch/arm/utility.cc Sun Jul 26 10:20:07 2015 -0500 >>> +++ b/src/arch/arm/utility.cc Sun Jul 26 10:21:20 2015 -0500 >>> @@ -156,6 +156,9 @@ >>> for (int i = 0; i < NumCCRegs; i++) >>> dest->setCCReg(i, src->readCCReg(i)); >>> >>> + // Copy vector registers when vector registers put to use. >>> + assert(NumVectorRegs == 0); >>> + >>> for (int i = 0; i < NumMiscRegs; i++) >>> dest->setMiscRegNoEffect(i, src->readMiscRegNoEffect(i)); >>> >>> diff -r e1309937d313 -r 5af8f40d8f2c src/arch/isa_parser.py >>> --- a/src/arch/isa_parser.py Sun Jul 26 10:20:07 2015 -0500 >>> +++ b/src/arch/isa_parser.py Sun Jul 26 10:21:20 2015 -0500 >>> @@ -515,6 +515,9 @@ >>> def isCCReg(self): >>> return 0 >>> >>> + def isVectorReg(self): >>> + return 0 >>> + >>> def isControlReg(self): >>> return 0 >>> >>> @@ -751,6 +754,106 @@ >>> >>> return wb >>> >>> +class VectorRegOperand(Operand): >>> + def isReg(self): >>> + return 1 >>> + >>> + def isVectorReg(self): >>> + return 1 >>> + >>> + def __init__(self, parser, full_name, ext, is_src, is_dest): >>> + ## Vector registers are always treated as source registers >>>since >>> + ## not the whole of them might be written, in which case we >>>need >>> + ## to retain the earlier value. >>> + super(VectorRegOperand, self).__init__(parser, full_name, ext, >>> + True, is_dest) >>> + self.size = 0 >>> + >>> + def finalize(self, predRead, predWrite): >>> + self.flags = self.getFlags() >>> + self.constructor = self.makeConstructor(predRead, predWrite) >>> + self.op_decl = self.makeDecl() >>> + >>> + if self.is_src: >>> + self.op_rd = self.makeRead(predRead) >>> + self.op_src_decl = self.makeDecl() >>> + else: >>> + self.op_rd = '' >>> + self.op_src_decl = '' >>> + >>> + if self.is_dest: >>> + self.op_wb = self.makeWrite(predWrite) >>> + self.op_dest_decl = self.makeDecl() >>> + else: >>> + self.op_wb = '' >>> + self.op_dest_decl = '' >>> + >>> + def makeConstructor(self, predRead, predWrite): >>> + c_src = '' >>> + c_dest = '' >>> + >>> + if self.is_src: >>> + c_src = '\n\t_srcRegIdx[_numSrcRegs++] = %s + >>> Vector_Reg_Base;' % \ >>> + (self.reg_spec) >>> + if self.hasReadPred(): >>> + c_src = '\n\tif (%s) {%s\n\t}' % \ >>> + (self.read_predicate, c_src) >>> + >>> + if self.is_dest: >>> + c_dest = '\n\t_destRegIdx[_numDestRegs++] = %s + >>> Vector_Reg_Base;' % \ >>> + (self.reg_spec) >>> + c_dest += '\n\t_numVectorDestRegs++;' >>> + if self.hasWritePred(): >>> + c_dest = '\n\tif (%s) {%s\n\t}' % \ >>> + (self.write_predicate, c_dest) >>> + >>> + return c_src + c_dest >>> + >>> + def makeRead(self, predRead): >>> + if self.read_code != None: >>> + return self.buildReadCode('readVectorRegOperand') >>> + >>> + vector_reg_val = '' >>> + if predRead: >>> + vector_reg_val = 'xc->readVectorRegOperand(this, >>> _sourceIndex++)' >>> + if self.hasReadPred(): >>> + vector_reg_val = '(%s) ? %s : 0' % \ >>> + (self.read_predicate, vector_reg_val) >>> + else: >>> + vector_reg_val = 'xc->readVectorRegOperand(this, %d)' % \ >>> + self.src_reg_idx >>> + >>> + return '%s = %s;\n' % (self.base_name, vector_reg_val) >>> + >>> + def makeWrite(self, predWrite): >>> + if self.write_code != None: >>> + return self.buildWriteCode('setVectorRegOperand') >>> + >>> + if predWrite: >>> + wp = 'true' >>> + if self.hasWritePred(): >>> + wp = self.write_predicate >>> + >>> + wcond = 'if (%s)' % (wp) >>> + windex = '_destIndex++' >>> + else: >>> + wcond = '' >>> + windex = '%d' % self.dest_reg_idx >>> + >>> + wb = ''' >>> + %s >>> + { >>> + TheISA::VectorReg final_val = %s; >>> + xc->setVectorRegOperand(this, %s, final_val);\n >>> + if (traceData) { traceData->setData(final_val); } >>> + }''' % (wcond, self.base_name, windex) >>> + >>> + return wb >>> + >>> + def makeDecl(self): >>> + ctype = 'TheISA::VectorReg' >>> + return '%s %s;\n' % (ctype, self.base_name) >>> + >>> class ControlRegOperand(Operand): >>> def isReg(self): >>> return 1 >>> @@ -818,7 +921,10 @@ >>> # Note that initializations in the declarations are solely >>> # to avoid 'uninitialized variable' errors from the compiler. >>> # Declare memory data variable. >>> - return '%s %s = 0;\n' % (self.ctype, self.base_name) >>> + if 'IsVector' in self.flags: >>> + return 'TheISA::VectorReg %s;\n' % self.base_name >>> + else: >>> + return '%s %s = 0;\n' % (self.ctype, self.base_name) >>> >>> def makeRead(self, predRead): >>> if self.read_code != None: >>> @@ -909,6 +1015,7 @@ >>> self.numFPDestRegs = 0 >>> self.numIntDestRegs = 0 >>> self.numCCDestRegs = 0 >>> + self.numVectorDestRegs = 0 >>> self.numMiscDestRegs = 0 >>> self.memOperand = None >>> >>> @@ -931,6 +1038,8 @@ >>> self.numIntDestRegs += 1 >>> elif op_desc.isCCReg(): >>> self.numCCDestRegs += 1 >>> + elif op_desc.isVectorReg(): >>> + self.numVectorDestRegs += 1 >>> elif op_desc.isControlReg(): >>> self.numMiscDestRegs += 1 >>> elif op_desc.isMem(): >>> @@ -1127,6 +1236,7 @@ >>> _______________________________________________ >>> gem5-dev mailing list >>> [email protected] >>> http://m5sim.org/mailman/listinfo/gem5-dev >> >> >> -- IMPORTANT NOTICE: The contents of this email and any attachments are >>confidential and may also be privileged. If you are not the intended >>recipient, please notify the sender immediately and do not disclose the >>contents to any other person, use it for any purpose, or store or copy >>the information in any medium. Thank you. >> >> ARM Limited, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, >>Registered in England & Wales, Company No: 2557590 >> ARM Holdings plc, Registered office 110 Fulbourn Road, Cambridge CB1 >>9NJ, Registered in England & Wales, Company No: 2548782 >> _______________________________________________ >> gem5-dev mailing list >> [email protected] >> http://m5sim.org/mailman/listinfo/gem5-dev >> >_______________________________________________ >gem5-dev mailing list >[email protected] >http://m5sim.org/mailman/listinfo/gem5-dev -- IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you. ARM Limited, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered in England & Wales, Company No: 2557590 ARM Holdings plc, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered in England & Wales, Company No: 2548782 _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
