Friday, 2018-03-23

*** tpb has joined #vtr-dev00:00
*** vtrbot has joined #vtr-dev00:04
*** digshadow has quit IRC00:58
*** digshadow has joined #vtr-dev02:18
*** digshadow has quit IRC04:58
*** digshadow has joined #vtr-dev06:58
*** digshadow has quit IRC11:38
mithromorning everyone!13:41
mithrokem_: Should my example arch+wire.eblif example work with latest head of vtr? I'm getting a different assertion failure now...13:42
mithrovtr-verilog-to-routing/vpr/src/place/timing_place_lookup.cpp:167 get_best_class: Assertion 'best_class >= 0 && best_class < type->num_class' failed.13:42
kem_mithro: It did work for me, but I've also seen that failure when I was trying to make a regression test to cover the original issue13:43
mithrokem_: I've been meaning to ask how I should be creating tests which fit into the vtr system13:44
kem_mithro: I added instructions to README.developers.md a couple weeks ago which give an overview13:45
mithrokem_: Okay great, I'll take a look at it13:45
daveshahkem_ and mithro: WRT https://github.com/mithro/vtr-verilog-to-routing/issues/5, where would be a good place to output attributes on locations? It looks like the .place file is the only option, but I'm not sure how useful that would be, or even how easy given the simple format of this file?13:45
mithrodaveshah: I would say the place file or the output verilog13:47
kem_mithro and daveshah: Its not entirely clear to me if tagging location attributes directly in VPR is the best approach. Can you give some insight into the motivation?13:49
mithrokem_: One big one is to able to tag the info which makes it possible to map vtr grid locations to real tile grid locations13:51
mithrokem_: but I can imagine in the future wanting vtr to do things based on the attributes -- for example I can imagine wanting to tag a block with "use only for route through"13:52
mithrokem_: I can imagine wanting it in the output verilog somehow13:54
daveshahAt the moment no placement information is dumped into the Verilog. It would be great to add this (optionally, in case older tools don't support attributes) as Verilog attributes too13:54
daveshahIt would also be moving towards allowing placement constraints in the long term13:55
kem_I completely buy the idea of location constraints. (I've actually drafted up a format to that end previously).13:56
kem_ But I think those are really 'design constraints', rather than 'architecture/device constraints' and hence shouldn't end up in the architecture file13:57
kem_Any thoughts?13:58
mithrokem_: Well - there are two seperate things - design constraints and architecture/device constraints13:58
mithrokem_: Real world device have all types of weird constraints like you can't use X if you use Y13:59
daveshahThere is also the issue that the locations in VPR may not be the same as the actual locations, as the user would expect and the vendor tools use - for example, because of the need for padding or because of only considering a region of interest in the 7-series at the moment13:59
mithroAn example of why we would want attributes on pb_types is to describe the functionality we are currently hard coding into the name https://github.com/SymbiFlow/symbiflow-arch-defs#names14:00
tpbTitle: GitHub - SymbiFlow/symbiflow-arch-defs: FOSS architecture definitions of FPGA hardware useful for doing PnR device generation. (at github.com)14:00
kem_mithro: I agree, it's just not clear to me where the architecture/device constraints should go. I would tend to lean towards trying to model the architecture correctly, rather than adding a (potentially unbounded) number of special cases.14:01
kem_Also, the co-ordinate transform between VPR and real device should be fairly straightforward to do externally to VPR shouldn't it?14:03
mithrokem_: It is when you have a 1-1 mapping between VPR tiles and device tiles -- but it can get really messy when you end up representing a tile in the real device as multiple tiles14:04
daveshahYes - the ECP5 for example can have situations like this, where multiple tiles occupy the same grid location14:05
daveshahhttps://usercontent.irccloud-cdn.com/file/4yyJ3FXA/ECP5%20tiles14:06
kem_Couldn't you just specify the 1x1 region in VPR's grid space to correspond to the smallest tile?14:07
kem_You'll end up with lots of blanks, but then the mapping would still be 1:1?14:08
*** sunxi_fan has quit IRC14:08
mithrokem_: Potentially14:09
daveshahkem_: Not sure whether or not that would create problems with channels though?14:10
kem_daveshah: I don't think it should be an issue if your loading the real-device RR graph14:12
kem_daveshah: you're*14:12
daveshahYes, I think that will be the best way forward for the foreseeable future14:12
mithrokem_: It can be really useful to run with a vpr generated virtual rr_graph which approximates the real graph14:13
kem_mithro: Agreed14:14
kem_mithro: And you would still be able to, it just might be a much coarser approximation14:14
mithrokem_: That is where things can get really messy with that approach14:14
kem_mithro: Valid point14:15
mithrokem_: I'd also like to try and keep a much of the architecture information inside the arch.xml file so we don't run into issues with files getting out of sync (plus it makes composability easier)14:16
mithrokem_: So that assert seems to be occuring because the tile doesn't have any RECEIVER pins...14:16
mithro    int sink_ptc = get_best_class(RECEIVER, device_ctx.grid[sink_x][sink_y].type);14:17
kem_mithro: A work around is to run with --timing_analysis off14:17
kem_mithro: The problem is in placement delay profiling14:18
kem_mithro: I was looking at it yesterday, and I know how to fix it but it requires refactoring the delay profiling to be more robust14:19
kem_mithro and daveshah: The issues you guys bring up are all definitely valid14:21
mithrokem_: Would a fix like this work -> https://github.com/mithro/vtr-verilog-to-routing/commit/c6692b04fa6ec1496a3bdde1911b54aa12a480a5 ?14:22
tpbTitle: Allow nodes without driver/receiver pins. · mithro/[email protected] · GitHub (at github.com)14:22
kem_mithro: Yes that's the first stage of the fix, but it turns out to break the delay look-up since some delta values may no longer be valid14:23
kem_mithro: Which requires re-working how we profile the delays to be more robust. I actually have most of the code to do this (written for other purposes)14:23
mithroWarning 3: Unable to route between blocks at (1,1) and (1,1) to characterize delay (setting to inf)14:24
mithroThat warning ends up with14:24
mithro /usr/local/google/home/tansell/work/catx/vtr-verilog-to-routing/vpr/src/timing/timing_util.cpp:588 calc_relaxed_criticality: Assertion 'crit >= 0. - CRITICALITY_ROUND_OFF_TOLERANCE' failed (Criticality should never be negative).14:24
kem_Yep thats the issue14:24
mithroIs that what you are referring too?14:24
mithroahh great - I've hit that issue a bunch of times before14:24
kem_It's not the most user friendly error message :(14:25
kem_But fundamentally it means that the placement delay lookup got an inf delay (since the delay profiler failed to find a valid route between the profile locations choosen for that delta x and delta y)14:26
kem_So the real fix is to make the profiling location selection more robust14:26
mithrookay14:26
kem_Coming back to the grid/attributes issue14:27
mithrokem_: Back on the topic of attributes - I feel like it is useful to be to generally be able to tag things with arbitrary metadata14:27
kem_mithro: I agree as well, in principle14:28
kem_mithro: But what I want to avoid is adding a large number of special cases which handicap the flexibility of the code base14:29
kem_mithro: I think there is a somewhat more fundamental issue here actually14:29
kem_mithro: Something I've been kicking around in the back of my mind14:29
mithrokem_: A agree on principle of trying to reduce special cases :-)14:30
kem_mithro: Since VPR's historic purpose has been architecture exploration, it's input specification (i.e. architecture file) is fundamentally under specified14:30
kem_mithro: And VPR has a bunch of code to build up a more complete device description according to that specification14:31
kem_mithro: Once that is done VPR's algorithms then map the design onto the complete device descritpion14:31
mithroYeap14:32
kem_mithro: For targeting real devices the interest is really in being able to specify that complete device description and using the impelementation algorithms14:32
kem_mithro: The problem is that expanding the flexibility of the architecture format to allow that level of specification makes it less flexible14:33
kem_mithro: That's actually somewhat of a tautological statement let me rephrase14:34
mithrokem_: Initially yes - I would however be very interested in being able to do things like take a real device and adding arbitrary extra restrictions on the top of it -- IE reducing the interconnect / routing available, etc14:34
kem_mithro: This is just an idea, but it may make sense to have an intermediate format which describes the 'complete' device architure14:35
kem_mithro: The rr_graph file is a partial implementation of this14:35
kem_mithro: If we had this 'complete' device specification file, it could be generated to match a real device from some device DB, or built by VPR from a higher level architecture specification14:36
kem_mithro: That would seem to be a logical place to put the types of attributes etc. that you're thinking of14:36
kem_mithro: Anyway, just a thought...14:37
mithrokem_: I'm not necessarily convinced that allowing architecture exploration and real world devices are necessarily conflicting14:37
mithrokem_: For example, while building up a real world device description it is super useful to have vpr "fill out" the rest of the device with non-real stuff14:38
mithrokem_: I would also be very interested in having a vpr mode which lets vpr "find the right device for your design"14:39
mithro(IE which part size is your design likely to fit into)14:39
kem_mithro: Those are all valid uses, and you may be right14:40
kem_mithro: I think the 'find the right device' is still pretty straight-forward if you expand 'complete device specification file' to 'complete device family specification file'14:40
mithrokem_: Another thing I'm very interested in is being able to give direct feedback to FPGA manufacturers about how they should change their design to better fit real world needs - IE if you just add a /little/ bit more fabric of this type it would enable me to use a much smaller part for the same design14:41
daveshahIt would also be able to have some kind of tiling ability, to minimise the amount of duplication and avoid having to have such a large file, for a device family specification file - unlike I believe how an rr_graph works at the moment for example14:42
mithrodaveshah: Yeah - I actually think the arch.xml is pretty good at reducing duplication at the moment14:45
kem_I agree exploiting tile-ability is definitely worth pursuing14:45
mithrokem_: So, it looks like I'm still getting rr_graph nodes with no edges :-/14:45
daveshahmithro: Yes, that's exactly the sort of thing I'm thinking of.  This goes back to the `sb_type` concept in #286 to some extent I feel14:45
daveshahBut would possibly go further than that14:46
mithrodaveshah / kem_: I actually think that the sb_type concept in #286 is general enough to describe both real world devices and to allow some very interesting academic exploration14:47
mithrodaveshah / kem_: But I'm sure once we have an implementation it will be obvious there is a much better way to do it :-P14:48
daveshahIt's also very similar to how I think Lattice Diamond is implemented internally, based on their physical viewer14:48
daveshahWhich is a good sign :)14:48
mithrohttps://www.irccloud.com/pastebin/4m1kPUMA/14:48
tpbTitle: Snippet | IRCCloud (at www.irccloud.com)14:48
kem_mithro: I guess that is part of my concern... it would be a lot of work to implement that type of specification and we'd want to be confident it was sufficiently general and not too difficult to use14:49
kem_mithro: Having an intermediate complete device specification would mean it could be targeted my multiple potential 'front-end' architecture specifications without causing a lot of churn in VPR14:50
kem_mithro: I'm not quite sure how to interpret that file format...14:51
mithrokem_: You mean the pastebin?14:51
kem_mithro: Yea14:51
kem_mithro: Is this coming from your RR graph generation tool?14:52
mithroX<grid x>Y<grid y>_<block name>[<ptc>].<rr_graph type>14:52
mithrokem_: Yes, I'm loading an rr_graph generated by vpr14:52
daveshahFYI - here is how the ECP5 routing and switchbox is drawn in Diamond14:53
kem_mithro: Can you point me to the architecture?14:53
daveshahhttps://usercontent.irccloud-cdn.com/file/UlcGbTio/ecp5%20switchbox.png14:53
daveshahI think this is very close to how the `sb_type` might work14:54
mithrokem_: Same one I pointed in the previous bug -> https://gist.github.com/mithro/ef23fac984eb229a0300bbda22fca6b6#file-arch-xml14:54
tpbTitle: Test Architecture for VPR · GitHub (at gist.github.com)14:54
mithrohttps://www.irccloud.com/pastebin/BlLWVyR8/14:55
tpbTitle: Snippet | IRCCloud (at www.irccloud.com)14:55
mithrokem_: That is the rr_graph.echo showing the same thing...14:55
kem_mithro: There are some corner cases which an leave things disconnected... I'll take a look14:56
mithroI wouldn't assume my arch definition isn't borked in some way :-P14:56
mithrohttps://www.irccloud.com/pastebin/j3hYZbR0/14:57
tpbTitle: Snippet | IRCCloud (at www.irccloud.com)14:57
mithroI think the issue is these muxes -> https://gist.github.com/mithro/ef23fac984eb229a0300bbda22fca6b6#file-arch-xml-L132-L13515:00
tpbTitle: Test Architecture for VPR · GitHub (at gist.github.com)15:00
mithroIt's definitely something to do with the inputs of the block15:05
mithrokem_: All SINK nodes should have at least one edge going into them, right?15:07
kem_mithro: I'd assume so, but I want to figure out which sink nodes they are first15:07
mithrohttps://gist.github.com/mithro/ef23fac984eb229a0300bbda22fca6b6#file-arch-xml-L85 I believe15:09
tpbTitle: Test Architecture for VPR · GitHub (at gist.github.com)15:09
mithrokem_: This looks like it is a bug in my code15:16
mithrokem_: I'm confused why the rr_graph.echo says zero edges though?15:16
kem_mithro: Ah OK now I think I understand15:17
kem_mithro: The RR graph is stored internally as a set of out-going edges associated with the source node15:18
kem_mithro: So SINKs have no fanout, hence no edges15:18
kem_mithro: The rr_graph.xml treats the edges as first-class elements listing both the source and sink15:18
mithrohttps://www.irccloud.com/pastebin/NJDhVAJl/15:20
tpbTitle: Snippet | IRCCloud (at www.irccloud.com)15:20
mithrokem_: So it seems like I just hit a bug in my code and a misunderstanding of the rr_graph.echo file15:23
kem_mithro: Glad its figured out. I've been sticking to the rr_graph.xml mostly now in part because it is a bit more explicit than the .echo15:25
kem_mithro: At some point we'll probably also drop the rr_graph.echo since it's superseded by the rr_graph.xml15:26
mithrokem_: Probably makes a lot of sense if you end up merging my rr_graph library as we can generate a bunch of human readable output from the rr_graph.xml file in Python15:26
mithrokem_: Gives you a lot more flexibility in how you output it too...15:27
kem_mithro: Yep potentially15:28
mithrokem_: Welp, back to seeing if I can manipulate this rr_graph file :-P15:30
daveshahkem_: I got the buildbot failure email for my commits. I'm not certain, but I think this may be because the Titan benchmarks it fetches need to be updated with the renamed `<fc>` attributes?16:48
*** digshadow has joined #vtr-dev16:57
mithrokem_: Would you be opposed to the vpr GUI responding to the arrow keys?16:59
kem_daveshah: Yes that makes sense. I'll take the action to fix that.17:00
daveshahkem_: Thanks!17:00
daveshahkem_: The arch file in GitHub is updated already, but I don't know if that's otherwise equivalent to the downloaded one?17:00
mithrokem_: One of my test FPGAs -> https://usercontent.irccloud-cdn.com/file/gknQOMRp/image.png17:01
daveshahmithro: BTW you can merge symbiflow-arch-defs #84 now17:01
kem_daveshah: It's different because the Titan benchmarks are too big to keep directly in the VTR tree, so they have an independent copy of the architecture file17:02
mithrodaveshah: I think I need to rebuilt vpr in conda first17:02
kem_mithro: Responding to arrow keys seems fine, although note that you can also use middle-clock via the mouse to pan around17:03
mithrokem_: That seems to zoom for me17:04
*** digshadow has quit IRC18:24
*** digshadow has joined #vtr-dev18:51
daveshahI think a possible next step, certainly before actually doing anything in archfpga/VPR, for the new `sb_type` specification would be to try and write a Python script or similar to generate `sb_type`s for various real FPGAs (at minimum 7-series, iCE40 and ECP5), and that may give a better idea as to whether it is worth proceeding with19:24
kem_mithro: Moving the mouse while holding middle-click should allow you to move around19:42
kem_daveshah: Proving out sb_type externally seems like a good approach19:43
*** digshadow has quit IRC21:16
kem_daveshah: I've updated the Titan release to fix the format changes, so BuildBot should now pass21:20
*** digshadow has joined #vtr-dev21:29
mithrokem_: Awesome thanks!21:30
daveshahkem_: I've started a rebuild to see what happens.21:32
kem_daveshah: One odd thing about buildbot is a 'rebuild' re-tests the same changeset -- so it likely won't see the fix I commited21:39
kem_daveshah: If we wait buildbot will see the new changeset and re-test21:40
mithrokem_: Wouldn't it be late for you? :-P21:50
daveshahOops - indeed it failed again but the proper build with your changes has started now22:02
daveshahkem_: Updating titan has fixed it, it's run successfully now, thanks!22:40

Generated by irclog2html.py 2.13.1 by Marius Gedminas - find it at mg.pov.lt!