----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/2119/#review4851 -----------------------------------------------------------
Ship it! Ship It! - Ali Saidi On Dec. 16, 2013, 3:55 p.m., Christopher Torng wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/2119/ > ----------------------------------------------------------- > > (Updated Dec. 16, 2013, 3:55 p.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > mips: Floating point convert bug fix > > Floating point convert instructions use the FloatConvertOp format defined > in src/arch/mips/isa/formats/fp.isa. The type of the operands in the ISA > description file (_sw for signed word, or _sf for signed float, etc.) is > used to create a type for the operand in C++. Then the operand is converted > using the fpConvert() function in src/arch/mips/utility.cc. > > Here's what an fp cvt looks like in the generated execution function in > build/MIPS/arch/mips/generated/inorder_cpu_exec.cc:: > > uint32_t val = Fs; > Fd = fpConvert(WORD_TO_SINGLE, val); > > The fpConvert function is in src/arch/mips/utility.cc:: > > uint64_t > fpConvert(ConvertType cvt_type, double fp_val) > { > > switch (cvt_type) > { > case SINGLE_TO_DOUBLE: > { > double sdouble_val = fp_val; > void *sdouble_ptr = &sdouble_val; > uint64_t sdp_bits = *(uint64_t *) sdouble_ptr; > return sdp_bits; > } > > case SINGLE_TO_WORD: > { > int32_t sword_val = (int32_t) fp_val; > void *sword_ptr = &sword_val; > uint64_t sword_bits= *(uint32_t *) sword_ptr; > return sword_bits; > } > > (...) > } > } > > Basically it does pointer casting to read values as a different type. > > The bug is that _val_ is unsigned instead of signed. So if we are converting > from > a word to a float, and we want to convert 0xffffffff, we expect -1 to be > passed into > fpConvert(). Instead, we see MAX_INT passed in. Then fpConvert() converts > _val_ to > MAX_INT in single-precision floating point, and we get the wrong value. > > To fix it, I changed the signs of the convert operands from unsigned to signed > in the MIPS ISA description. > > Then, I changed the FloatConvertOp format to insert a int32_t into the C++ > code instead > of a uint32_t. > > > Diffs > ----- > > src/arch/mips/isa/decoder.isa bdd606534bdc > src/arch/mips/isa/formats/fp.isa bdd606534bdc > > Diff: http://reviews.gem5.org/r/2119/diff/ > > > Testing > ------- > > I've tested this in two ways: 1) by writing a MIPS assembly test that > converts a test set of words to floats, and 2) in a C++ isolation test. > > **MIPS assembly test** > > I've compiled some handwritten assembly like this:: > > TEST( cvt.s.w, 0x4372a3d5, 0x4e86e548 ) > TEST( cvt.s.w, 0xffffffff, 0xbf800000 ) > > .. where TEST is a test macro that uses the instruction in arg0 with the > data in arg1. > The reference output is in arg2. > > The objdump looks like this for each test:: > > 808000: 3c024372 lui v0,0x4372 > 808004: 3442a3d5 ori v0,v0,0xa3d5 > 808008: 46801120 cvt.s.w a0,v0 > 80800c: 3c014e86 lui at,0x4e86 > 808010: 3421e548 ori at,at,0xe548 > 808014: 14810095 bne a0,at,808270 <_fail> > > $V0 gets the src data. $A0 gets the fp-converted data. $AT gets the > reference data. > We branch to fail code if they don't match. > > All negative number tests fail without this patch. > > **C++ testing** > > I copied the fpConvert() function and the ConvertType enum to a separate C++ > test file to test in isolation. > > fptest.cc:: > > #include <iostream> > #include <stdint.h> > > //used in FP convert & round function > enum ConvertType{ > SINGLE_TO_DOUBLE, > SINGLE_TO_WORD, > SINGLE_TO_LONG, > > DOUBLE_TO_SINGLE, > DOUBLE_TO_WORD, > DOUBLE_TO_LONG, > > LONG_TO_SINGLE, > LONG_TO_DOUBLE, > LONG_TO_WORD, > LONG_TO_PS, > > WORD_TO_SINGLE, > WORD_TO_DOUBLE, > WORD_TO_LONG, > WORD_TO_PS, > > PL_TO_SINGLE, > PU_TO_SINGLE > }; > > uint64_t > fpConvert(ConvertType cvt_type, double fp_val) > { > > switch (cvt_type) > { > case SINGLE_TO_DOUBLE: > { > double sdouble_val = fp_val; > void *sdouble_ptr = &sdouble_val; > uint64_t sdp_bits = *(uint64_t *) sdouble_ptr; > return sdp_bits; > } > > case SINGLE_TO_WORD: > { > int32_t sword_val = (int32_t) fp_val; > void *sword_ptr = &sword_val; > uint64_t sword_bits= *(uint32_t *) sword_ptr; > return sword_bits; > } > > case WORD_TO_SINGLE: > { > float wfloat_val = fp_val; > void *wfloat_ptr = &wfloat_val; > uint64_t wfloat_bits = *(uint32_t *) wfloat_ptr; > return wfloat_bits; > } > > case WORD_TO_DOUBLE: > { > double wdouble_val = fp_val; > void *wdouble_ptr = &wdouble_val; > uint64_t wdp_bits = *(uint64_t *) wdouble_ptr; > return wdp_bits; > } > > default: > std::cout << "ERROR" << std::endl; > return 0; > } > } > > int main(int argc, const char *argv[]) > { > uint32_t test_value_original = 0xffffffff; > int32_t test_value_patched = 0xffffffff; > > uint64_t converted_value_original = fpConvert(WORD_TO_SINGLE, > test_value_original); > uint64_t converted_value_patched = fpConvert(WORD_TO_SINGLE, > test_value_patched); > > std::cout << std::hex << converted_value_original << std::endl; > std::cout << std::hex << converted_value_patched << std::endl; > > return 0; > } > > In main, I print out the fpConvert'ed values for 0xffffffff inputs wrapped > inside uint32_t and int32_t. The output is:: > > 4f800000 > bf800000 > > 0xBF800000 is the expected value (-1 in single precision floating point). > 0x4F800000 is MAX_INT in single precision floating point -- not what we > want. > > > Thanks, > > Christopher Torng > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
