Pnr sort

[steveri]

Trying to clean up redundant code...

[steveri]

@yuchen-mei @jack-melchert

Yuchen, I'm trying to sort out some code that you wrote and merged in pull https://github.com/StanfordAHA/garnet/pull/1025 , with respect to some earlier code that I wrote that had tried and perhaps failed to do the same thing.

Please look at my code change in this pull, and see if you agree that it is still the same functionality as your original code.

Also: if you think the pnr_wrapper() indirection is confusing, we can get rid of it.

Also: it looks like the existing code now does the following:

              GARNET.PY FLAGS                       PNR CALL
    --pipeline-pnr   --generate-bitstream-only   place_and_route()
    ------------------------------------------   -----------------
        True               True                         1
        True               False                      1 + 2
        False              True                         1
        False              False                        1

Where
1 => call place_and_route() with 
       unconstrained_io = (args.unconstrained_io or args.generate_bitstream_only),
       load_only = args.generate_bitstream_only

2 => call place_and_route() with 
       unconstrained_io = True,
       load_only = True

In other words, if --pipeline-pnr is True and there is no --generate-bitstream-only, place_and_route() gets called twice...I'm guessing that is not the desired behavior? I'm thinking the table should look more like?

GARNET.PY FLAGS
    --pipeline-pnr   --generate-bitstream-only   place_and_route()
        True               True                       1
        True               False                      2
        False              True                       1
        False              False                      1

Although I'm not exactly sure. Since this seems to have something to do with pipelining, I'm thinking maybe Jack can help us figure out what's supposed to be going on...?

Also: does the CI test this behavior someplace, so that we know it is still correct when we modify it?

[steveri]

Looking more closely at the truth table, I'm thinking we could get by with a single place_and_route() call where

unconstrained_io = args.pipeline or args.generate_bitstream_only or args.unconstrained_io
load_only = args.pipeline or args.generate_bitstream_only

I show my work below, you may or may not find it coherent or useful...in case it's not obvious, uio is unconstrained_io and genbo is generate_bitstream_only like...

     GARNET.PY FLAGS             PLACE_AND_ROUTE() CALL PARAMS
    --pipeline     --genbo    |          uio                  load_only
    --------------------------+----------------------------------------------
        True         True     |  args.uio or args.genbo    args.genbo (True)
        True         False    |         True                  True
        False        True     |  args.uio or args.genbo    args.genbo (True)
        False        False    |  args.uio or args.genbo    args.genbo (False)

=>
    --pipeline     --genbo    |    uio                      load_only
    --------------------------+----------------------------------------------
        True         True     |  args.uio             args.genbo or args.pipeline
        True         False    |  True                 args.genbo or args.pipeline
        False        True     |  True                 args.genbo or args.pipeline
        False        False    |  False                args.genbo or args.pipeline

=>
    --pipeline     --genbo    |  unconstrained_io (uio)    load_only
    --------------------------+----------------------------------------------
        True         True     |  args.(pl or genbo or uio)   args.genbo or args.pipeline
        True         False    |  args.(pl or genbo or uio)   args.genbo or args.pipeline
        False        True     |  args.(pl or genbo or uio)   args.genbo or args.pipeline
        False        False    |  args.(pl or genbo or uio)   args.genbo or args.pipeline

[jack-melchert]

Hey Steve, before we merge this in, ill take a close look today and see if everything looks good.

[jack-melchert]

Just for clarity and future reference, this is how the flow used to work:

        placement, routing, id_to_name, instance_to_instr, \
            netlist, bus = garnet.place_and_route(
                args.app, args.unconstrained_io or args.generate_bitstream_only, compact=args.compact,
                load_only=args.generate_bitstream_only,
                pipeline_input_broadcasts=not args.no_input_broadcast_pipelining,
                input_broadcast_branch_factor=args.input_broadcast_branch_factor,
                input_broadcast_max_leaves=args.input_broadcast_max_leaves)

        if args.pipeline_pnr and not args.generate_bitstream_only:
            # Calling clockwork for rescheduling pipelined app
            import subprocess
            import copy
            cwd = os.path.dirname(args.app) + "/.."
            cmd = ["make", "-C", str(cwd), "reschedule_mem"] 
            env = copy.deepcopy(os.environ)
            subprocess.check_call(
                cmd,
                env=env,
                cwd=cwd
            )

            placement, routing, id_to_name, instance_to_instr, \
                netlist, bus = garnet.place_and_route(
                    args.app, True, compact=args.compact,
                    load_only=True,
                    pipeline_input_broadcasts=not args.no_input_broadcast_pipelining,
                    input_broadcast_branch_factor=args.input_broadcast_branch_factor,
                    input_broadcast_max_leaves=args.input_broadcast_max_leaves)

The idea is that the garnet.place_and_route call doesn't actually always do all of place and route. The load_only flag will instead just call both archipelago PNR code that loads in an existing place and route result, and garnet.load_netlist which loads in the configuration of the tiles.

Therefore here are the list of behaviors that I expect from the generate_bitstream_only and pipeline_pnr flags:

1. pipeline=False and genbo=False This will call garnet.place_and_route(load_only=False) which does place and route, but no rescheduling.

2. pipeline=False and genbo=True This will call garnet.place_and_route(load_only=True) which does no actual placement and routing, it just loads a previous PnR result and the configuration of all the tiles.

3. pipeline=True and genbo=False This will call garnet.place_and_route(load_only=False), then reschedule_pipelined_app, then garnet.place_and_route(load_only=True). The second place_and_route call will update the id_to_name and instance_to_instr variables.

4. pipeline=True and genbo=True This is the same as 2.

You are absolutely correct that somewhere an extra call to garnet.place_and_route got added. Your updated code looks great!

Thanks Steve

[steveri]

@jack-melchert thanks so much for your thoughtful reply. I kind of thought that was what happened and mea culpa, I'm pretty sure I'm the one that messed it up with my pnr_wrapper() indirection. At the risk of messing up again, I'm going to do another rewrite that should simplify everything, before finalizing the pull.

[steveri]

Hey Steve, before we merge this in, ill take a close look today and see if everything looks good.

Yes, please. I think I'm done tweaking it, I was just about to request a new review of the final changes.

I removed the pnr_wrapper() indirection and pushed the args into the body of the place_and_route() function, where they are used. Also there was a conflict between arg defaults and parm defaults, I inserted an explainer comment that you can see in the code...let me know if you have questions. Thanks very much!

[steveri]

Oops forgot to merge this branch and now I had to fix conflicts. As soon as the conflict-resolution checks have passed, I will merge...I think I got all the final approvals, yes?