----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/2828/#review6384 -----------------------------------------------------------
Hi, I'm Giacomo and I've been working on ARMv8 support in the past (including NEON), and I'd like to share some thoughts on this patch. 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... 3. I guess adding vectors could enable implementing AVX-512, and potentially it could also enable research into more wacky "vector-style" architectures, which means that potentially the vectors could be REALLY wide. For this reason, I'm a bit concerned about passing around all those vectors by value. How about changing the register access API to return by ref when accessing vectors? (not quite sure about the implications of that on the rest of the code base though, but I think we'd need to somehow address the efficiency problem). Thanks, and sorry for the long review! - Giacomo Gabrielli On May 17, 2015, 9:56 p.m., Nilay Vaish wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/2828/ > ----------------------------------------------------------- > > (Updated May 17, 2015, 9:56 p.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > Changeset 10838:2dd0278fd3b1 > --------------------------- > cpu: implements vector registers > > This patch aims at adding vector registers type. The type is defined > as a std::array of some 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. > > The x86 isa files have been modified so that x86 can have instructions > with vector operands. > > > Diffs > ----- > > src/arch/SConscript 9b424e7adac5 > src/arch/alpha/registers.hh 9b424e7adac5 > src/arch/arm/registers.hh 9b424e7adac5 > src/arch/isa_parser.py 9b424e7adac5 > src/arch/mips/registers.hh 9b424e7adac5 > src/arch/null/registers.hh 9b424e7adac5 > src/arch/power/registers.hh 9b424e7adac5 > src/arch/sparc/registers.hh 9b424e7adac5 > src/arch/x86/insts/static_inst.hh 9b424e7adac5 > src/arch/x86/insts/static_inst.cc 9b424e7adac5 > src/arch/x86/isa.hh 9b424e7adac5 > src/arch/x86/isa/microasm.isa 9b424e7adac5 > src/arch/x86/isa/operands.isa 9b424e7adac5 > src/arch/x86/memhelpers.hh 9b424e7adac5 > src/arch/x86/nativetrace.hh 9b424e7adac5 > src/arch/x86/nativetrace.cc 9b424e7adac5 > src/arch/x86/registers.hh 9b424e7adac5 > src/arch/x86/regs/vector.hh PRE-CREATION > src/arch/x86/x86_traits.hh 9b424e7adac5 > src/cpu/StaticInstFlags.py 9b424e7adac5 > src/cpu/base_dyn_inst.hh 9b424e7adac5 > src/cpu/checker/cpu.hh 9b424e7adac5 > src/cpu/checker/cpu_impl.hh 9b424e7adac5 > src/cpu/exec_context.hh 9b424e7adac5 > src/cpu/o3/O3CPU.py 9b424e7adac5 > src/cpu/o3/cpu.hh 9b424e7adac5 > src/cpu/o3/cpu.cc 9b424e7adac5 > src/cpu/o3/dyn_inst.hh 9b424e7adac5 > src/cpu/o3/free_list.hh 9b424e7adac5 > src/cpu/o3/regfile.hh 9b424e7adac5 > src/cpu/o3/regfile.cc 9b424e7adac5 > src/cpu/o3/rename_map.hh 9b424e7adac5 > src/cpu/reg_class.hh 9b424e7adac5 > src/cpu/simple/base.hh 9b424e7adac5 > src/cpu/simple_thread.hh 9b424e7adac5 > src/cpu/static_inst.hh 9b424e7adac5 > src/sim/insttracer.hh 9b424e7adac5 > > Diff: http://reviews.gem5.org/r/2828/diff/ > > > Testing > ------- > > > Thanks, > > Nilay Vaish > > _______________________________________________ gem5-dev mailing list gem5-dev@gem5.org http://m5sim.org/mailman/listinfo/gem5-dev
