calyxir / calyx

Intermediate Language (IL) for Hardware Accelerator Generators
https://calyxir.org
MIT License
453 stars 45 forks source link

Fxdpoint dat #292

Closed yoonachang closed 3 years ago

yoonachang commented 3 years ago

I have extended only for convert2dat not convert2json yet. json data file for non fixed points are expected to look like { "mem0": { "data": [ [5,0,0,0], [7,0,0,0], [22,0,0,0], [3,0,0,0] ], "bitwidth": 8, "fixedpoint": [] }, "mem1": { "data": [ [9,0,0,0], [32,0,0,0], [80,0,0,0], [24,0,0,0] ], "bitwidth": 8, "fixedpoint": [] } }

and json data file for fixed points are expected to look like { "mem0": { "data": [ [5.5,0,0,0], [7.0,0,0,0], [2.5,0,0,0], [3.25,0,0,0] ], "bitwidth": 8, "fixedpoint": [3,5] }, "mem1": { "data": [ [9.0,0,0,0], [3.0,0,0,0], [8.25,0,0,0], [24.5,0,0,0] ], "bitwidth": 8, "fixedpoint": [3,5] } }

For fixedpoints I'm basically converting it into the right fixed-point representation in binary string and then converting it back to the format that the dat accepts. For example, 9.0 in mem1 would be converted to 20

I tried to write test cases to see if it works for the whole system, which are inttest and inttest2 which are intended to just add the elements in the corresponding address of mem0 and mem1 and write the resulting in mem2. But the results in mem2 are

rachitnigam commented 3 years ago

Talked to @yoonachang about using a "type" field in the JSON data field. Holding off the review till the updates become available next week.

cgyurgyik commented 3 years ago

Just wanted to say thanks Yoona, I very much look forward to this!

rachitnigam commented 3 years ago

This looks like its going in the right direction. For the two tests that are currently disable, I seem to get the wrong answers:

⇒  fud e tests/correctness/memaddfxd.futil --to dat -s verilog.data tests/correctness/memaddfxd.futil.data
{
  "a0_00_0":[[3.5,2.25],[4.125,2.5]],
  "b0_00_0":[[7,4],[6,2]],
  "result0_00_0":[[2.5,6.25],[2.125,4.5]]
}

For example, it says 3.5 + 7 = 2.5.

The next step is figuring out which step is producing the wrong value. For example:

  1. Does the converter generate the right representation for the input?
  2. Does the output bit representation from FuTIL look correct?
  3. Is the output correctly converted into decimal?

All of these can be answered by carefully inserting print statement in the python code. Also, one other thing that could be worth looking into: how are you handling signed vs unsigned representation? Signed numbers are represented using two's complement while unsigned numbers are not.

Let me know when you've figured out why the answers for both the tests are wrong.

rachitnigam commented 3 years ago

Separate note: Let's be careful before merging this. It will break our benchmarking setup.

yoonachang commented 3 years ago

For the example that is outputting wrong answers, I enforced it so that fixedpoint numbers always requires "." so 7 should be "7.0" I don't know why that is working, anyways. I haven't considered for signed operations yet so I'll start there to fix the problem. Would signed integer also require special accommodations too?

yoonachang commented 3 years ago

Also, a lot of runt tests seem to fail after I pulled from git. It's saying there's no module named 'packaging', do you know what is happening?

rachitnigam commented 3 years ago

My guess is you’re missing a Python package? I recommend googling around to see if there is a package named that. You can also do flit install -s from the fud directory

yoonachang commented 3 years ago

thanks, doing flit install -s worked.

yoonachang commented 3 years ago

This looks like its going in the right direction. For the two tests that are currently disable, I seem to get the wrong answers:

⇒  fud e tests/correctness/memaddfxd.futil --to dat -s verilog.data tests/correctness/memaddfxd.futil.data
{
  "a0_00_0":[[3.5,2.25],[4.125,2.5]],
  "b0_00_0":[[7,4],[6,2]],
  "result0_00_0":[[2.5,6.25],[2.125,4.5]]
}

For example, it says 3.5 + 7 = 2.5.

The next step is figuring out which step is producing the wrong value. For example:

  1. Does the converter generate the right representation for the input?
  2. Does the output bit representation from FuTIL look correct?
  3. Is the output correctly converted into decimal?

All of these can be answered by carefully inserting print statement in the python code. Also, one other thing that could be worth looking into: how are you handling signed vs unsigned representation? Signed numbers are represented using two's complement while unsigned numbers are not.

Let me know when you've figured out why the answers for both the tests are wrong.

1)The error 3.5 + 7 = 2.5 is being caused because the data example is "ufix<8,3>" there is an overflow when representing 10.5 because the integer part should be 3 bits which can't represent integer 10. I should change the bits for the example. I'm thinking of throwing an error for this case where overflows happen. Does this sound alright?

2)I found out signed integers/fxd points aren't yet implemented which was great that you pointed out so I'll work on it.

rachitnigam commented 3 years ago

1)The error 3.5 + 7 = 2.5 is being caused because the data example is "ufix<8,3>" there is an overflow when representing 10.5 because the integer part should be 3 bits which can't represent integer 10. I should change the bits for the example. I'm thinking of throwing an error for this case where overflows happen. Does this sound alright?

Did changing the bits make the example work or not?

2)I found out signed integers/fxd points aren't yet implemented which was great that you pointed out so I'll work on it.

Cool

yoonachang commented 3 years ago

Did changing the bits make the example work or not?

Fud is still outputting zeros on my side, I tried cloning again and connecting fud to that directory but it still doesn't work so I just tested it by directly running the python code with 10.5. I'm not completely sure, but I think it will work now. (Any suggestions on other things I would try to fix fud?)

2)I found out signed integers/fxd points aren't yet implemented which was great that you pointed out so I'll work on it.

I've pushed implementation of this

rachitnigam commented 3 years ago

Okay, how about you try deleting all installations of Dahlia and FuTIL from your computer and re-installing everything from scratch? You need to be able to run tests locally on your machine.

The answers are still wrong:

correctness dynamic (2 tests)
  ? miss - tests/correctness/smemaddfxd.futil
         ~
        1│+ {
        2│+   "a0_00_0": [
        3│+     [
        4│+       3.5,
        5│+       2.25
        6│+     ],
        7│+     [
        8│+       4.125,
        9│+       2.5
       10│+     ]
       11│+   ],
       12│+   "b0_00_0": [
       13│+     [
       14│+       [
       15│+         -3.25,
       16│+         4.0
       17│+       ],
       18│+       [
       19│+         1.0,
       20│+         -2.0
       21│+       ]
       22│+     ]
       23│+   ],
       24│+   "result0_00_0": [
       25│+     [
       26│+       -6.75,
       27│+       6.25
       28│+     ],
       29│+     [
       30│+       5.125,
       31│+       -4.5
       32│+     ]
       33│+   ]
       34│+ }
       35│+ 
         ~

2 missing
rachitnigam commented 3 years ago

The parsing implementation is getting a bit hairy. What if we try using a library like this one: https://github.com/francof2a/fxpmath.

rachitnigam commented 3 years ago

@yoonachang @sgpthomas Can we try to merge this before the monday meeting? If this is too out of date, we should just rewrite it instead of delaying the merge.

rachitnigam commented 3 years ago

Also, presumably, the fixedpoint directory should be deleted once we merge this.

cgyurgyik commented 3 years ago

Also, presumably, the fixedpoint directory should be deleted once we merge this.

One note on this: In #404, I store my table generator for fixed point exp there. Perhaps there is a better place? In any case, I think it's good to keep around especially if we eventually want to produce a std_fp_exp for any width.

sampsyo commented 3 years ago

Maybe primitives (i.e., where the library code currently goes) would be OK? I know this would be different because it's the generator code rather than pure Calyx/SystemVerilog code, but nonetheless it seems thematically appropriate.

rachitnigam commented 3 years ago

Yeah, primitives is better.

yoonachang commented 3 years ago

@rachitnigam what do you mean by rewriting? I can look over the code during the weekend but I'm just confused what I should do to finish this off and merge the code.

rachitnigam commented 3 years ago

Hey @yoonachang, I just meant if there are too many merge conflicts, we can try rebasing it to the current master. More generally, one of me or @sgpthomas have to stare hard that this PR to make sure things make sense before merging.

yoonachang commented 3 years ago

@rachitnigam Okay, thank you for the clarification.

cgyurgyik commented 3 years ago

Yeah, primitives is better.

Moved in #404. It can be deleted here.