Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DPI-C structures seem to come out backwards! #1191

Closed
veripoolbot opened this issue Aug 17, 2017 · 10 comments
Closed

DPI-C structures seem to come out backwards! #1191

veripoolbot opened this issue Aug 17, 2017 · 10 comments
Assignees

Comments

@veripoolbot
Copy link

@veripoolbot veripoolbot commented Aug 17, 2017


Author Name: Rob Stoddard
Original Redmine Issue: 1191 from https://www.veripool.org

Original Assignee: Wilson Snyder (@wsnyder)


In my module I have:


  // -----------------------------------------------------------------------------
  typedef struct {
         int  ip_nrst;
         int  ei_fs_clk;
         int  ei_dsp_clk;
         int  m_drate;
         int  m_dcoef;
         int  i_voice;
         int  i_voice_stb;
         int  eo_dfs_clk;
         int  o_band;
         int  o_band_stb;
     }  iostruct;

  // -----------------------------------------------------------------------------

  import "DPI-C" function integer c_IP_name (inout iostruct ios);

     iostruct ios;
         always @(negedge ip_nrst
                 or negedge ei_fs_clk
                 or posedge ei_fs_clk
                 or negedge ei_dsp_clk
                 or posedge ei_dsp_clk
         ) begin
       // assign inputs to C task structure inputs:
       ios.ip_nrst     = 1;
       ios.ei_fs_clk   = 2;
       ios.ei_dsp_clk  = 3;
       ios.m_drate     = 4;
       ios.m_dcoef     = 5;
       ios.i_voice     = 6;
       ios.i_voice_stb = 7;
       ios.eo_dfs_clk  = 8;
       ios.o_band      = 9;
       ios.o_band_stb  = 10;
       // call the C task model:
       c_IP_name (ios);

</code>

And in my C code I have:


int c_IP_name(iostruct* ios)
{
> printf("ip_nrst = %d\n", ios->ip_nrst);
> printf("fs__clk = %d\n", ios->ei_fs_clk);
> printf("dsp_clk = %d\n", ios->ei_dsp_clk);
> printf("m_drate = %d\n", ios->m_drate);
> printf("m_dcoef = %d\n", ios->m_dcoef);
> printf("i_voice = %d\n", ios->i_voice);
> printf("i_v_stb = %d\n", ios->i_voice_stb);
> 
> printf("dfs_clk = %d\n", ios->eo_dfs_clk);
> printf("o_band  = %d\n", ios->o_band);
> printf("o_b_stb = %d\n", ios->o_band_stb);
> return 0;
}

</code>

Which is using the structure, in C:


typedef struct {
     int ip_nrst;
     int ei_fs_clk;
     int ei_dsp_clk;
     int m_drate;
     int m_dcoef;
     int i_voice;
     int i_voice_stb;
     int eo_dfs_clk;
     int o_band;
     int o_band_stb;
}  iostruct;

</code>

Basically identical.

But when run, I get this output:

ip_nrst = 10
fs__clk = 9
dsp_clk = 8
m_drate = 7
m_dcoef = 6
i_voice = 5
i_v_stb = 4
dfs_clk = 3
o_band  = 2
o_b_stb = 1

Which is backwards from what I'd expect.

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Aug 17, 2017


Original Redmine Comment
Author Name: Rob Stoddard
Original Date: 2017-08-17T23:30:18Z


Oh yeah...

Using built-in specs.
COLLECT_GCC=/usr/x86_64-pc-linux-gnu/gcc-bin/5.4.0/gcc
COLLECT_LTO_WRAPPER=/usr/libexec/gcc/x86_64-pc-linux-gnu/5.4.0/lto-wrapper
Target: x86_64-pc-linux-gnu
Configured with: /var/tmp/portage/sys-devel/gcc-5.4.0-r3/work/gcc-5.4.0/configure --host=x86_64-pc-linux-gnu --build=x86_64-pc-linux-gnu --prefix=/usr --bindir=/usr/x86_64-pc-linux-gnu/gcc-bin/5.4.0 --includedir=/usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include --datadir=/usr/share/gcc-data/x86_64-pc-linux-gnu/5.4.0 --mandir=/usr/share/gcc-data/x86_64-pc-linux-gnu/5.4.0/man --infodir=/usr/share/gcc-data/x86_64-pc-linux-gnu/5.4.0/info --with-gxx-include-dir=/usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5 --with-python-dir=/share/gcc-data/x86_64-pc-linux-gnu/5.4.0/python --enable-languages=c,c++ --enable-obsolete --enable-secureplt --disable-werror --with-system-zlib --enable-nls --without-included-gettext --enable-checking=release --with-bugurl=https://bugs.gentoo.org/ --with-pkgversion='Gentoo Hardened 5.4.0-r3 p1.3, pie-0.6.5' --enable-esp --enable-libstdcxx-time --enable-shared --enable-threads=posix --enable-__cxa_atexit --enable-clocale=gnu --enable-multilib --with-multilib-list=m32,m64 --disable-altivec --disable-fixed-point --enable-targets=all --disable-libgcj --enable-libgomp --disable-libmudflap --disable-libssp --disable-libcilkrts --disable-libmpx --enable-vtable-verify --enable-libvtv --disable-libquadmath --enable-lto --without-isl --disable-libsanitizer
Thread model: posix
gcc version 5.4.0 (Gentoo Hardened 5.4.0-r3 p1.3, pie-0.6.5)

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Aug 17, 2017


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2017-08-17T23:49:29Z


Sounds like you might be looking at this yourself, but either way can you please convert this to a test_regress format standalone test? That will be needed either for me to debug, or to take a patch to know it's good. Thanks.

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Aug 18, 2017


Original Redmine Comment
Author Name: Rob Stoddard
Original Date: 2017-08-18T15:05:56Z


I created a regression test (see patch) which reflects the issue. One this this regression does not show, however, is that all of the integers in the struct are properly formed; so the solution is not as simple as rolling over the bits. Furthermore, this issue may behave completely differently on big-endian machines... in fact, I suspect the issue does not exist on them. I do not have one at hand that I can use to test.

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Aug 24, 2017


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2017-08-24T02:50:54Z


I believe the correct behavior is what you indicated with your "this code works" section, which is what Verilator currently does. VCS agrees with Verilator. IEEE says the first structure packed entity is at the highest indicies, so I'm not seeing how your alternative could be right. What simulator disagrees?

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Aug 24, 2017


Original Redmine Comment
Author Name: Rob Stoddard
Original Date: 2017-08-24T13:43:12Z


This leaves me deeply concerned with how VCS works... that may be a bug. Modelsim certainly disagrees with Verilator.

Note that in the "this code works" the operation is reversed with respect to the Verilog. In the test, I use the exact-same structure definition in Verilog as I do in C, so the general assumption should be that what I put in through Verilog is the same thing I get in C.

When I submit two elements in a structure, I use the non-commutative property of subtraction to verify the positioning of the two elements in the structure. I provided code in Verilog that subtracts the two input elements, a - b, and in C, a - b, then compares the result. The "this code works" comment shows that swapping the two elements (b - a) creates the correct answer, which indicates the elements of the structure are swapped.

To put it in other terms, when I submit a structure to a DPI-C function, the elements within the structure are in reverse order when they get into C. So if I submit a structure in Verilog:

typedef struct 
{
     int first;
     int second;
     int third; 
} MyStruct_t;

</code>

With the values:

MyStruct_t my_struct;
my_struct.first = 1; 
my_struct.second = 2; 
my_struct.third = 3; 
</code>

Then in C, I define the exact same struct (omitted... it's identical to the Verilog definition) The following statements are true:

my_struct.first == 3;
my_struct.second == 2;
my_struct.third == 1;
</code>

Clearly reversed. So, I decided since Google was not being nice to me and providing the appropriate results, I'd create a little C test program:

#include <stdio.h>

typedef struct
{
         int a;
         int b;
         int c;
         int d;
} MyStruct;

typedef union
{
         MyStruct a;
         int b[4];
} MyUnion;

int main()
{
         MyUnion x;
         int i;
         for(i = 0; i < 4; i++)
                 x.b[i] = i;
         printf("A = %d\n", x.a.a);
         printf("B = %d\n", x.a.b);
         printf("C = %d\n", x.a.c);
         printf("D = %d\n", x.a.d);

         return 0;
}
</code>

Which gave me the following results:

A = 0
B = 1
C = 2
D = 3
</code>

It appears that C's structure has the indices in order, first is lowest in memory, and then up from there.

I am looking at IEEE 1800-2012, section 7.2, and I don't see any direct mention of structure order for either packed or unpacked structures. Is it mentioned elsewhere?

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Aug 24, 2017


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2017-08-24T23:08:35Z


Did the exact test you sent pass on modelsim, that is does "test_regress/t/t_dpi_import.pl --ms" pass? I suspect you're comparing a non-packed struct on modelsim with a packed struct on VCS/Verilator.

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Aug 28, 2017


Original Redmine Comment
Author Name: Udi Finkelstein
Original Date: 2017-08-28T08:08:32Z


There seems to be 2 issues here:

Section 7.2.1 indicates:
"A packed structure can be used as a whole with arithmetic and logical operators. The first member specified is the most significant and subsequent members follow in decreasing significance."

This means that within SystemVerilog, your packed structure will have it's first elements in the high bits, and the last element in the lowest bits. Think of it like the '{...}' concatenation operator in Verilog.

As to how SystemVerilog treats structures passed as arguments to DPI functions, are they copied as-is or are structure members mapped (because obviously C orders structure members the opposite way) ?

Looking at chapter 35 (DPI) I see the following:

Section 35.2.2.1 says this:
"DPI does not add any constraints on how SystemVerilog-specific data types are actually implemented. Optimal representation can be platform dependent. The layout of 2- or 4-state packed structures and arrays is implementation and platform dependent."

Later below, on 35.5.6 it says:
"Packed arrays, structs, and unions composed of types bit and logic. Every packed type is eventually equivalent to a packed one-dimensional array. On the foreign language side of the DPI, all packed types are perceived as packed one-dimensional arrays regardless of their declaration in the SystemVerilog code".

I'm not sure how to interpret that, but taking the SystemVerilog struct, casting it into a bit vector and then passing this as an array pointer to C, seems to be the most logical thing to do.
In you case, you cast it back to a struct, which happened to have it's members reversed due to the way Systemverilog treats the order of struct members when converting them to a bit vector.

Recently I wrote a small script (~300 lines of perl code) at works that takes an external .h file containing simple byte/short/int/enum types in various struct/union combinations, and converts it into the equivalent SystemVerilog header file. This way a CPU can DMA a data structure and the SystemVerilog code parsing it in hardware can share a SystemVerilog struct with the equivalent C struct automatically.

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Aug 29, 2017


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2017-08-29T02:41:53Z


You didn't indicate if it passed in modelsim. Anyhow I also tried Cadence and it too agrees with Verilator, and besides as you found it's supposedly implementation dependent (though I believe that's silly in this case). Either way I believe Verilator is compliant. I commited your test with the Verilator passing result.

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Aug 30, 2017


Original Redmine Comment
Author Name: Rob Stoddard
Original Date: 2017-08-30T18:24:28Z


Is the functionality I'm looking for in unpacked structures?

I was having a sincere problem getting the example working in Modelsim. I sent it to one of my designers, he said he got it working however, he never returned code to me nor provided me with results... he's busy in other things, as am I.

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Aug 30, 2017


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2017-08-30T21:55:24Z


Is the functionality I'm looking for in unpacked structures?

Yes, though no idea how widely supported that is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.