WIP: Adding structure to represent a give node/context point in RTI tree. #1299
Conversation
|
I think this is a good idea.
I will do a review.
You can already submit your documentation changes. Making many smaller PR is always easier to handle than a big one.
|
src/grt/grt-rtiis.ads
Outdated
| -- Just in here so we have a constant that we can harmlessly include | ||
| -- include in another package to force compilation. | ||
| FISH : constant Natural := 2; | ||
|
|
There was a problem hiding this comment.
You don't need that. You can always do:
with Grt.Rtiis;
pragma Unreferenced (Grt.Rtiis);
src/grt/grt-rtiis.ads
Outdated
| -- from the Rti and the Ctxt, but we also store it here so we don't have | ||
| -- to determine it too often. | ||
| Layout_Addr : Address; | ||
| -- If the Base_Rti is unbound then this is the address of the |
There was a problem hiding this comment.
Maybe create: subtype Layout_Address is Address; to clarify the use of the address ?
src/grt/grt-rtiis.ads
Outdated
| -- be instatiated many times in the design, an RTII node | ||
| -- represents one specfic instance of that node. | ||
| type Ghdl_Rtii is record | ||
| -- Whether it is a signal/port or a generic/constant. |
There was a problem hiding this comment.
I fear it is confusing to have both Ghdl_Rtii and Ghdl_Rtii_Type. What about Ghdl_Object_Rtii and Ghdl_Type_Rtii ?
src/grt/grt-rtiis.ads
Outdated
|
|
||
| -- Get a indexed child of an Rtii. | ||
| function Get_Rtii_Child(Rtii : Ghdl_Rtii; Index : Ghdl_Index_Type) | ||
| return Ghdl_Rtii; |
There was a problem hiding this comment.
There are some style differences: space before '(', return aligned, 2 spaces after '--'.
There was a problem hiding this comment.
Do you have a formatting checker you're using in emacs?
src/grt/grt-rtiis.adb
Outdated
| Base_Record := To_Ghdl_Rtin_Type_Record_Acc( | ||
| To_Ghdl_Rtin_Subtype_Composite_Acc(Rtii.Typ.Rti).Basetype); | ||
| when others => | ||
| Internal_Error("get_record_child"); |
There was a problem hiding this comment.
Create a function to get the base type of a record type/subtype.
src/grt/grt-rtiis.adb
Outdated
| -- Get the length of this dimension so we can check whether | ||
| -- the index is valid. | ||
| Idx_Rti := Get_Base_Type (Base_Array.Indexes (Rtii.Typ.Dim)); | ||
| Extract_Range (Bounds_Addr, Idx_Rti, Rng); |
There was a problem hiding this comment.
Maybe the multi-dim arrays should be flattened (as if they had only one dimension). That could simplify the code.
There was a problem hiding this comment.
Yep. I need to check how this is handled in VPI and VHPI and then try to stay close to that.
src/grt/grt-rtiis.adb
Outdated
| Layout_Addr := Rtii.Typ.Layout_Addr; | ||
| when others => | ||
| Internal_Error("get_array_child"); | ||
| end case; |
There was a problem hiding this comment.
You should create a function to get layout_addr and base_array from the type rtii.
|
Thanks for taking the time to look at this. |
|
Not a complete one in emacs. I follow the indentation given by emacs but it doesn't deal with anything else.
|
|
I've got lots of bugs to fix here :). I expect I'll have something ready for you to take a look at again by the end of next weekend. |
|
That works for me. Thanks!
|
|
Any hints on why the llvm & gcc tests are failing? |
|
Those errors:
```
C:/projects/ghdl-psgys/src/grt/grt-rtiis.adb:44: undefined reference to `system__concat_3__str_concat_3'
5916C:\Tools\GHDL\master-1212-mingw64-llvm\lib/ghdl/libgrt.a(grt-rtiis.o): In function `grt__rtiis__get_array_child':
5917C:/projects/ghdl-psgys/src/grt/grt-rtiis.adb:198: undefined reference to `system__img_uns__image_unsigned'
```
You are using string concatenation and 'image. There are implemented using functions from the Ada library, but the executable generated with llvm and gcc backend is not linked with the Ada library.
For 'image, you can use grt-images
For concatenation, you'd better to avoid it or do it manually.
If this is just debug code then it doesn't matter.
|
|
Makes sense. Thanks. |
|
I've made most of the changes you suggested. The only one I haven't implemented so far was adding a This branch is passing all the tests, which I think is more likely to indicate gaps in the tests, rather than that this is ready to merge :). My main concern is that I will break the generation of wave dumps. To check this I'm going to add a test that creates wave dumps and then confirm that these wave dumps are unchanged when the test is run in this branch. The main change in this branch since last time that you looked at it, is that I'm now using the RTII structure in the Avhpi handle. It's ready for you to take a look at it now if you'd like, or you can wait until I've tested it more thoroughly. |
|
As suspected this pull request is currently totally breaking the generation of wave file dumps. |
|
Just an update, since I haven't made any changes to this issue for ages. I've been using this branch at work for the last month without issues, so it likely doesn't break too much. |
|
So I've haven't worked on this for ages, and have mostly forgotten where I was at. What I do remember was that it was really, really hard, and I wasn't making very much progress. I felt like I was having to reverse engineer the grt structure because it wasn't clearly defined anywhere. I think a first step is probably making the grt more comprehensible. I'd be curious to hear what people with a better understanding of ghdl think the proper approach would be. |
|
Much of what I was doing was creating structures to capture the runtime information. But I wasn't touching any of the code related to actually creating that data in the first place. I think a more sensible approach would be to use the same structures both for creating the runtime information in the first place, and for accessing it later for use with VPI or VHPI. |
|
Do you mean grt structures or RTI structures. I suppose the latter.
I agree they aren't simple to deal with.
They represent the tree at runtime. But this is a waste of efforts, as the tree is well described by the AST at compile time, and finally the RTI is just a different dump of the AST (with some extra data). So basically, we need to maintain both the AST and the RTIs.
On top of that, to deal with dynamic elaboration, the RTIs have a mix of offsets and of absolute addresses and we need to deal with the complexity of the offsets at runtime.
A different option would be to only use JIT (or code generation in the same process). So the AST would be available and just have to be decorated with info (addresses/offsets) from the code generation. No more RTIs. I think that would simplify VPI or VHPI.
@Blebowski did some contributions related to the RTIs. You can ask his feelings as a GHDL contributor.
|
|
@benreynwar thanks for bringing this discussion. Let me ping @marlonjames, who's been working on VHPI. |
|
My (very slow) incremental work on the VHPI implementation hasn't taken me this far yet, so I don't have any useful input. I agree that it's better to avoid dual maintenance. |
|
Currently there's not only dual maintenance between the AST and the RTI, but also between where the RTIs are created and the various places that they are used. There's a lot of logic about interpreting the various absolute and offset addresses that is repeated independently and is confusing. It would be nice if we could capture all of the logic about the various kinds of offsets in one place. That's kind of what I was trying to do last year with the RTII structure, but I was only looking at making sense of the data after it had already been constructed in memory. It would also be nice to share structures with the code that is setting up the memory in the first place. I'm probably missing a lot of the subtlety.
This sounds like a big change. @tgingold, you can much better judge whether this is worth it. Do you think it is? |
|
This sounds like a big change. @tgingold, you can much better judge whether this is worth it. Do you think it is?
It is a big change, and I think it is worth it (or at least worth investigating it). But that won't happen soon.
|
|
Some might already know, but others might be mislead by my activity. Let me clarify that I'm no expert on the LRM, on Ada or on GHDL internals. The AST, RTI, GRT, etc. are really blurry in my mind. Hence, please excuse me if any of the following comments don't make sense.
My perception is that this feeling is shared by all the people that tries to develop some tool on top of GHDL:
I know not all of those areas are using exactly the same types/tree. But all of them need to somehow understand either the AST or the RTI or some variation/subset of both.
It seems that you are proposing some heavy internal refactoring with no other initial purpose than reshaping the codebase. I believe that can be a very exciting activity for several of us to try helping. @Blebowski has been doing some arbitrary cleanups in this sense, at his own will. Ben, Tristan, maybe any of you can point us to where should we look for "cleaning up the code that is setting up the memory"? Related to Tristan's comment about JIT, my perception is that all that offset nightmare is not only a matter of code cleanness, but might also be forced by the complexity of supporting two compilation strategies. For context, the discussion about switching GHDL to a JIT only solution was brought previously in the context of top-level generics. See #1388. As Tristan commented, switching to JIT would imply getting rid of the GCC backend/build. However, LLVM supports JIT. When we last talked about it, there were two main features missing from LLVM which were supported by GCC: debugging and code coverage. Since then, debugging is supported with LLVM too. Yet, code coverage is still possible with GCC only, and that's the main reason for many users of GHDL to pick that backend, specially in aero-space-mil industries. Therefore, a nice strategy would be to investigate how to do coverage with LLVM and document/advertise it for moving the user base to mcode and LLVM only. As @Blebowski commented in #1695, that would reduce the maintenance burden. |
Currently the RTI codebase is somewhat difficult to work on because there are lots of factors (RTI node structure, context, bounds location, whether it's in a signal or generic) that are needed to properly describe a point in the RTI tree.
In this pull request I added an RTII (Run Time Instance Information) structure that gathers this data into a single structure. These structures will be generated dynamically as required. Avhpi handles for signals/ports/generics/constants and their subcomponents will contain one of these RTII structures.
My hope is that this will make the codebase simpler to understand, and easier to implement new features.
This is not a branch that should be merged, I've just put in this pull request to get some feedback on the idea of creating this structure, and to get suggestions for alternative ways of approaching this.
My current plan is: