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

General code issues thread #108

Open
TaraHoleInIt opened this issue Jan 11, 2021 · 57 comments
Open

General code issues thread #108

TaraHoleInIt opened this issue Jan 11, 2021 · 57 comments

Comments

@TaraHoleInIt
Copy link

I'll start the first one:

In my attempt to debug WaitForRisingEdge by passing in a debug LED I now receive the following error message:

^^^^^^^^^^^^^^^^^^^ incorrect input parameters in call to algorithm 'WaitForRisingEdge', last correct match was parameter 'Signal'

I've checked the documentation and examples a few times, and its not immediately obvious why this is coming up.

$$Period = 1 / Frequency;
$$NSPerClock = 1000000000 * Period;
$$ClocksPerMS = math.floor( 1000000 / NSPerClock );

algorithm Debounce( input uint1 Source, output uint1 Dest ) <autorun> {
	uint24 Clocks = 0;
	uint8 Bitmap = 0;

	while ( 1 ) {
		Clocks = Clocks + 1;

		if ( Clocks == $ClocksPerMS$ ) {
			Bitmap = ( Bitmap << 1 ) | Source;
			Clocks = 0;
		}

		// If all the bits are set then the button should have finished bouncing
		// and the output should be reliable.
		Dest = ( Bitmap == 8hff ) ? 1 : 0;
	}
}

subroutine WaitForRisingEdge( input uint1 Signal, output uint1 Debug ) {
	uint1 Last = 0;

	Debug = 1;

	while ( 1 ) {
		if ( Last == 0 && Signal == 1 ) {
			break;
		}

		Last = Signal;
	}

	Debug = 0;
}

algorithm main( output uint$NUM_LEDS$ leds, output uint$NUM_GPIO_OUT$ gp, input uint$NUM_GPIO_IN$ gn ) {
	uint1 FilteredButton = 0;
	uint1 Button = 0;
	uint1 State = 0;

	Debounce Btn(
		Source <: Button,
		Dest :> FilteredButton
	);

	Button := gn[ 10,1 ];
	leds[ 0,1 ] := State;

	while ( 1 ) {
		// Error
		// ^^^^^^^^^^^^^^^^^^^ incorrect input parameters in call to algorithm 'WaitForRisingEdge', last correct match was parameter 'Signal'
		( ) <- WaitForRisingEdge <- ( FilteredButton, LED );
		State = ~State;
	}
}
```
@sylefeb
Copy link
Owner

sylefeb commented Jan 11, 2021

The syntax for the subroutine call is:
(output0,...,outputN) <- sub <- (input0,....,inputK)
So the line has to be changed for
(LED) <- WaitForRisingEdge <- ( FilteredButton)

Something else, more related to timing -- in this part:

   Clocks = Clocks + 1;
   if ( Clocks == $ClocksPerMS$ ) {
	// ...
   }

It is best to increment Clock after the if (adjusting the logic accordingly). This will help timing as the test and addition will no longer depend on each other within the same cycle. (I started writing guidelines for this, hoping to finish soon). In this case this will have little impact, but it is a good habit to take.

(Edit:) the same is true for Dest = ( Bitmap == 8hff ) ? 1 : 0; which is used in the condition after being updated in the if. Btw, this could also be written with an always assign, writing Dest := ( Bitmap == 8hff ) ? 1 : 0; above the while(1) (this will effectively assign Dest first thing every clock cycle). In this case it does not really impact, but that can be very convenient if Dest has to be constantly updated regardless of the state of the algorithm.

@TaraHoleInIt
Copy link
Author

Thank you for the feedback!
I will keep this in mind for the future.

That being said, I'm still having trouble getting WaitForRisingEdge to work:

subroutine WaitForRisingEdge( input uint1 Signal ) {
	uint1 Exit = 0;
	uint1 Last = 0;

	Last = Signal;

	while ( Exit == 0 ) {
		Exit = ( Last == 0 && Signal == 1 ) ? 1 : 0;
		Last = Signal;
	}
}

algorithm main( output uint$NUM_LEDS$ leds, output uint$NUM_GPIO_OUT$ gp, input uint$NUM_GPIO_IN$ gn ) {
	uint1 FilteredButton = 0;
	uint1 Button = 0;
	uint1 State = 0;
	uint1 ADebug = 1;

	Debounce Btn(
		Source <: Button,
		Dest :> FilteredButton
	);

	Button := gn[ 10,1 ];
	leds[ 0,1 ] := ADebug;

	gp[ 0,1 ] := State;

	while ( 1 ) {
		ADebug = 1;
			( ) <- WaitForRisingEdge <- ( FilteredButton );
		ADebug = 0;

		State = ~State;
	}
}

I'm not sure if its a code problem or FPGAs being a bit unfamiliar.
As far as I can tell it never actually returns, but FilteredButton is behaving just as I would expect.

@sylefeb
Copy link
Owner

sylefeb commented Jan 13, 2021

I think I see the problem. When you call a subroutine it copies its arguments - precisely to avoid them changing while it processes. This is why WaitForRisingEdge does not see FilteredButton change.

Two possible approaches. 1) Incorporate the subroutine in the algorithm (which they are meant to be), so you can actually access the algorithm internals. This becomes:

algorithm main( output uint$NUM_LEDS$ leds, output uint$NUM_GPIO_OUT$ gp, input uint$NUM_GPIO_IN$ gn ) {

	uint1 FilteredButton = 0;
	uint1 Button = 0;
	uint1 State = 0;
	uint1 ADebug = 1;

        subroutine WaitForRisingEdge( reads FilteredButton ) {
	        uint1 Exit = 0;
	        uint1 Last = 0;
        
	        Last = FilteredButton ;
        
	        while ( Exit == 0 ) {
		        Exit = ( Last == 0 && FilteredButton == 1 ) ? 1 : 0;
		        Last = FilteredButton;
	        }
        }


	Debounce Btn(
		Source <: Button,
		Dest :> FilteredButton
	);

	Button := gn[ 10,1 ];
	leds[ 0,1 ] := ADebug;

	gp[ 0,1 ] := State;

	while ( 1 ) {
		ADebug = 1;
			( ) <- WaitForRisingEdge <- ( );
		ADebug = 0;

		State = ~State;
	}
}

Note the explicit declaration reads FilteredButton ; this is to make it clear what a subroutine is supposed to manipulate.

The other option is to turn WaitForRisingEdge into another algorithm and use parameter bindings:

algorithm WaitForRisingEdge( input uint1 FilteredButton ) {
  uint1 Exit = 0;
  uint1 Last = 0;

  Last = FilteredButton ;

  while ( Exit == 0 ) {
    Exit = ( Last == 0 && FilteredButton == 1 ) ? 1 : 0;
    Last = FilteredButton;
  }
}

algorithm main( output uint$NUM_LEDS$ leds, output uint$NUM_GPIO_OUT$ gp, input uint$NUM_GPIO_IN$ gn ) {

	uint1 FilteredButton = 0;
	uint1 Button = 0;
	uint1 State = 0;
	uint1 ADebug = 1;

	Debounce Btn(
		Source <: Button,
		Dest :> FilteredButton
	);

        WaitForRisingEdge waitedge(
            FilteredButton <: FilteredButton, // parameter binding
        );

	Button := gn[ 10,1 ];
	leds[ 0,1 ] := ADebug;

	gp[ 0,1 ] := State;

	while ( 1 ) {
		ADebug = 1;
		() <- waitedge <- (); // no input parameter as it is bound
		ADebug = 0;

		State = ~State;
	}
}

When a parameter is bound it tracks the value of the source (connected through a wire in terms of hardware). Note that there are two bindings: <: tracks the value as it is updated during the cycle, <:: tracks the value at it was at the cycle starts (this is a topic I will document better, it is a very interesting degree of freedom to play with in designs). In short, <: is typical and allows the algorithm to immediately start refreshing the outputs when a value is changed -- but this can also reduce the max frequency of the design as the signal propagation deepens. <:: will trigger a refresh only at the next cycle: pay one in latency but increase max freq.

@TaraHoleInIt
Copy link
Author

I have done some testing with generating delays, it seems like ones done inline are accurate while wrapping them in a subroutine causes problems.

For example, if I have a counter in my main loop and toggle a pin whenever the counter reaches 4 then I get a 250ns signal which is what I would expect from a 16MHz clock.
Trying to make this neater by wrapping it up in a subroutine is nowhere near as accurate.

Maybe I'm just approaching this wrong and there is a more correct way to accurately handle timing?

@sylefeb
Copy link
Owner

sylefeb commented Jan 19, 2021

Hi - the timing would have to account for the subroutine entry/exit cycles. This is partially discussed in Section 5 but I really have to revise this with more examples and diagrams, that is an important topic.

So for instance, let's consider this wait subroutine in a small test case:

subroutine wait(input uint16 incount)
{
  uint16 count = uninitialized;
  count = incount;
  while (count != 0) { count = count - 1; }
}
	
algorithm main( output uint$NUM_LEDS$ leds,input uint1 btns ) 
{
  uint16 cycle = 0;

  always { cycle = cycle + 1; }

  __display("[before] cycle %d",cycle);
  () <- wait <- (0);
  __display("[next  ] cycle %d",cycle);
  () <- wait <- (4);
  __display("[done  ] cycle %d",cycle);
}

Now simulating with icarus make icarus (Makefile at the end) we get:

[before] cycle     0
[next  ] cycle     4
[done  ] cycle    12

So we can see the subroutine call, even with a 0 parameter takes 4 cycles. With an input of 4 it then takes 8 cycles. So there is an overhead of four: 2 for entry/exit of subroutine and 2 for the entry/exit of the while. Note that there are chain rules, so adding a while right after the other will add a single cycle instead of 2 (clarifying the documentation right now!).

What if we want a less than 4 cycles wait? We can use multiple ++: operators, or use a circuitry to inline a while loop with a parameter for its duration.

Taking these delays into account should give you the exact timing.

.DEFAULT: test.ice
		silice-make.py -s test.ice -b $@ -p basic -o BUILD_$(subst :,_,$@)

clean:
	rm -rf BUILD_*

@TaraHoleInIt
Copy link
Author

TaraHoleInIt commented Jan 20, 2021

Ohhhh okay!
I think I see what was happening then.

I don't think I was taking into account not only that entering and exiting the subroutine were taking cycles, but that the main loop was adding some delay as well.
That being said, nothing I'm doing now requires timing that precise, I just wanted to understand the behaviour I was seeing.

Thank you :)

@at91rm9200
Copy link

Hello,

I am a Silice and FPGA newbie and have a question regarding readwrite permissions on subroutines.

In the following example the subroutine out_led is called by the subroutine out_led_inverted. For this, the subroutine out_led_inverted needs to get permission to access the parameter pattern from out_led.

algorithm main(output uint5 leds) {

uint26 cnt = 0;

   subroutine out_led(input uint5 pattern, readwrites leds) {
      leds = pattern;
   }

   subroutine out_led_inverted(input uint5 pattern_inv, readwrites i_out_led_pattern) {
   //                                                              ^^^^^^^^^^^^^^^^^
      () <- out_led <- (~pattern_inv);
   }

   while(1) {
      cnt = cnt + 1;
      () <- out_led_inverted <- (cnt[21,5]);
   }
} 

But the syntax for this is not clear to me. Only from the compiler error message "variable 'i_out_led_pattern' is written by subroutine 'out_led_inverted' without explicit permission" I could recognize the name for this parameter. Is this intentional or did I miss something?

Regards, Bernd.

sylefeb added a commit that referenced this issue Dec 25, 2021
@sylefeb
Copy link
Owner

sylefeb commented Dec 25, 2021

Hello Bernd,

The error message is indeed not useful, I'll improve that. The subroutine out_led_inverted has to explicitly declare it intends to call out_led. Here is how to do that:

subroutine out_led_inverted(input uint5 pattern, calls out_led) {

Welcome to Silice!
Best regards,
Sylvain

@sylefeb
Copy link
Owner

sylefeb commented Dec 25, 2021

Improved error message (in wip branch):

() <- out_led <- (~pattern)
^^^^^^^^^^^^^^^^^^^^^^^^^^^ subroutine 'out_led_inverted' calls other subroutine 'out_led' without permssion
                            add 'calls out_led' to declaration if that was intended.

error: Silice compiler stopped

Thanks for pointing this out!!

@at91rm9200
Copy link

Hello Sylvain,

thank you for the explanation. It works perfectly.

Regards, Bernd.

@at91rm9200
Copy link

Hello,

I am trying to bind two pmod bits, one as input and the other one as output, to an algorithm. The input binding "bit_in <: pmod.i[0,1]" seems to work, but the output binding "bit_out :> pmod.o[1,1]" leads to an error: "cannot find accessed base.member 'pmod.o'". For sure, this is a beginner question. But I cannot see, how to solve this.

algorithm alg_test(input uint1 bit_in, output uint1 bit_out)
{
   bit_out = ~bit_in;
}

algorithm main(output uint5 leds, inout uint8 pmod) {

   alg_test a(bit_in <: pmod.i[0,1], bit_out :> pmod.o[1,1]);  // <-- cannot find accessed....

   while (1) {
      pmod.oenable = 8b00000010;
      () <- a <- ();
   }
}  

Regards, Bernd.

@sylefeb
Copy link
Owner

sylefeb commented Feb 1, 2022

Hi Bernd,

Your syntax makes perfect sense, unfortunately you've hit a limitation on output bindings (see #208). The error message is not helpful and I'll improve that shortly (ultimately the syntax your are using will be supported).

The good news is, there are simple workarounds, see https://github.com/sylefeb/Silice/blob/draft/tests/issues/108_pmod.ice

Thanks for the report!
Sylvain

@sylefeb
Copy link
Owner

sylefeb commented Feb 1, 2022

Partially fixed the issue:

  • The error message is now more informative:
pmod.o[1,1]
^^^^^^^^^^^ binding an output to a partial variable (bit select, bitfield, table entry) is currently unsupported

Changes in draft branch.

@at91rm9200
Copy link

Hello Sylvain,

thank you for the answer, for improving the Silice compiler and providing the workaround.
I think, I am going to stick with the master branch for now.

Regards, Bernd.

@at91rm9200
Copy link

Hello Sylvain,

can an inout pmod simply be used as a gpio? The following program should output four pulses on the icestick pmod (bit 0) and then read bit 1 (pulled up via a resistor) and output it on the LED. The program only works if the marked step is commented out. If the step is not commented out, then bit 1 is apparently not read in correctly. I suspect I am missing something significant, or may be the inout pmods can not be used this way?

algorithm main(output uint5 leds, inout uint8 pmod) {
   uint3 ctr = 4;
   uint1 pmod_bit1 = uninitialized;

   pmod.oenable:= 8b00000001;
   pmod.o = 8b00000001;

   while (ctr > 0) {
      ctr = ctr - 1;
      ++:                       // <--- works, if this step is commented out.
      pmod.oenable = 8b00000001;
      pmod.o = 8b00000001;
      ++:
      pmod.oenable = 8b00000001;
      pmod.o = 8b00000000;
   }
   ++:
   pmod.oenable = 8b00000001;
   pmod_bit1 = pmod.i[1,1];     // pmod bit 1 is pulled high via resistor.

   leds[0,1] = pmod_bit1;

   while(1) {
   }
}  

Regards, Bernd.

@sylefeb
Copy link
Owner

sylefeb commented May 30, 2022

Hi Bernd - thanks for the report, I am looking into it! (I see the same behavior and it is indeed confusing).

@sylefeb
Copy link
Owner

sylefeb commented May 30, 2022

I believe I have identified the issue -- it's not obvious, and I am actually surprised by the behavior. Nevertheless I decided to modify the tristate so that the behavior is stable. This means introducing a one cycle latency between when .o / .oenable are set and the actual change on the pin, but that is anyway better in terms of managing IO skew/delays.

The change is applied in the wip branch ; I will soon merge with master but a few adaptations remain to be done in other projects. Please let me know if you have any trouble.

(Side note: instead of while (ctr > 0) use while (ctr != 0) to reduce LUT usage ;) An even better way is to make ctr signed and test whether the sign bit is set (meaning -1 is reached), adjusting the counter accordingly).

@at91rm9200
Copy link

Hello Sylvain,

thanks for tracking this down. I think I will wait for the master branch.
But I have one more question about the above example: The I/O configuration of the pmod does not change in the algorithm. It is always 8b00000001, seven inputs and one output in the same place. Shouldn't a single always assignment (pmod.oenable:= 8b00000001) be enough? Or does pmod.oenable have to be explicitly set before each pmod.o and pmod.i statement, as I did above?

Regards, Bernd.

@sylefeb
Copy link
Owner

sylefeb commented May 30, 2022

Hi Bernd, the single assignment should indeed be enough (and is enough after the fix), the best being a single pmod.oenable := 8b00000001; since the := will hold the value at all times (unless overridden by the algorithm), allowing for a simpler logic. But even a single pmod.oenable = 8b00000001; at the top (without the :=) would do now.

The fact you needed multiple assignments was a side effect of the problem itself.

@at91rm9200
Copy link

Hello Sylvain,

thank you for the explanation. I have come across the strange "pmod behavior" several times. I'm glad you were able to fix it.

Regards, Bernd.

@sylefeb
Copy link
Owner

sylefeb commented May 31, 2022

Thanks a lot Bernd for the tests + reports (inouts are used in fewer designs, and tristates can be a bit tricky...). In general feel free to reach out if anything seems suspicious, I am happy to check.

@at91rm9200
Copy link

Hello Sylvain,

I am having difficulties to get the dual ported BRAM working. The following design for the Icestick should use ICESTORM_RAM, but if I look into the log-file (next.log) I can see a utilization of 0 % for the RAM. I have the "simple_dualport_bram" working in another design, but this time I want to read from both ports.

unit main(output uint5 leds) {

   dualport_bram uint8 mem[128] = {0,1,2,3,4,5,6,7,8,9,pad(uninitialized)};
   uint7 ctr = 0;

   algorithm {

      while(1) {
         mem.wenable0 = 0;       // read port 0
         mem.addr0 = ctr;
         ++:
         leds = mem.rdata0;

         mem.wenable1 = 0;       // read port 1
         mem.addr1 = ctr + 1;
         ++:
         leds = mem.rdata1;

         ctr = ctr + 1;
      }
   }
}
Info: Device utilisation:
Info: 	         ICESTORM_LC:   170/ 1280    13%
Info: 	        ICESTORM_RAM:     0/   16     0%
Info: 	               SB_IO:     6/  112     5%
Info: 	               SB_GB:     3/    8    37%
Info: 	        ICESTORM_PLL:     0/    1     0%
Info: 	         SB_WARMBOOT:     0/    1     0%

I am not using the very latest version of yosys (0.20 ), nextpnr (0.3) and Silice.

It would be nice if you could check if I did something wrong.

Regards, Bernd.

@sylefeb
Copy link
Owner

sylefeb commented Dec 31, 2022

Hi Bernd,

Just had a quick look, this is indeed odd. As a workaround comment mem.wenable0 = 0; and it will synthesize using brams again (tested on Yosys 0.17+76). As long as you only read on port 0 this is fine because the default (on fpga) will be 0 anyway. That actually means using the bram as a simple_dualport_bram that allows read/writes on port 1 and only reads on port 0.

I'll investigate further. Thanks for the report!

@at91rm9200
Copy link

Hello Sylvain,

thank you. This works. I can read now over port 0 and read/write over port 1.
As far as I can see, using dualport_bram (vs. simple_dualport_bram) seems to double the resources in the FPGA concerning ICESTORM_RAM. This would halve the amount of BRAM. However.

Bonne année.

Regards, Bernd.

@at91rm9200
Copy link

Hello Sylvain,

the draft version behaves differently than the master version regarding local variables in subroutines. The master version compiles the following design without problems, the draft version throws an error in line 9:

unit main(output uint5 leds, inout uint8 pmod)
{
   uint3 ctr = 0;

   algorithm
   {
      subroutine delay(input uint24 cycle_counter)
      {
         uint24 ctr = 0;   // <----- Line 9: variable 'ctr': this name is already used by a prior declaration

         ctr = cycle_counter;
         while (ctr > 0) {
            ctr = ctr - 1;
         }
      }

      while (1) {
         leds = leds + 1;
         ctr = ctr + 1;
         () <- delay <- (2000000);
         if (ctr == 0) {
            leds = 0;
         }
      }
   }

}
 

Is this a problem of the draft version?

Regards, Bernd.

@sylefeb
Copy link
Owner

sylefeb commented Mar 28, 2023

Hi Bernd - thanks for the report. Indeed this changed in draft but has to be revised. In general it is best to avoid shadowing a variable name, but I intend to issue a 'strong' warning as opposed to an error on such cases (and there are other similar cases that needs clarifying too). Sorry for the trouble, will try to look into it soon! (Opened new issue #250)

@sylefeb
Copy link
Owner

sylefeb commented Apr 24, 2023

Hi Bernd, this problem (name shadowing) should be fixed in master, please let me know in case of trouble.

@at91rm9200
Copy link

Hello Sylvain,

I am trying to realize an 8 bit bidirectional data transfer with an ICE40 FPGA. The FPGA should read the external data with the falling edge of the clock.
I found the module projects/common/ice40_sb_io_inout.v, which uses the SB_IO primitive to get the desired pin functions. Analog to the project spiflash2x.si, I tried to create the pin bindings without success:

import('ice40_sb_io_inout.v')

unit main(output uint5 leds,
          inout uint8 sdr_dq)

{
   uint8 io_oe(0); uint8 io_i(0); uint8 io_o(0);

   sb_io_inout sb_io0(clock <: clock,
                      oe <: io_oe[0,1],
                      in :> io_i[0,1],
                      out <: io_o[0,1],
                      pin <:> sdr_dq[0,1]);
                      ^^^^^^^^^^^^^^^^^^^ expecting an identifier on the right side of a group binding

   algorithm {
      while (1) {
      }
   }
}         

Obviously I am doing something wrong.
Regards, Bernd.

@sylefeb
Copy link
Owner

sylefeb commented Dec 30, 2023

Hi Bernd,

Sorry for the delay in answering. There is a limitation on tristate pins, they cannot be bound with a bit select (so the issue is sdr_dq[0,1]). Instead you either have to:

This is not great, I'll look into fixing this limitation! (opened issue #258). The error message also needs clarification.

Thanks and best wishes for 2024!
Sylvain

@at91rm9200
Copy link

ok, I am going to take the breakout route then. So I can avoid Verilog.

Have a good new Year.
Bernd.

@sylefeb
Copy link
Owner

sylefeb commented Jan 17, 2024

Hi Bernd, I have lifted this restriction (in draft branch), so that binding inouts with bit select is now possible. Please let me known if there's any trouble with it.
Best,
Sylvain

@at91rm9200
Copy link

Hello Sylvain,

I am pleased that you have removed the bit select restriction and would like to try out the new version.

But I have problems to compile the draft version of Silice. The master version compiles without problems.
I am still using Ubuntu 18.04 with gcc version 9.4.0 and cmake 3.22.1. If you say its too old, then I am going to update my build system. Otherwise:

cmake throws a lot of warnings like:

-- Detecting CXX compile features - done
CMake Deprecation Warning at antlr/antlr4-cpp-runtime-4.7.2-source/CMakeLists.txt:2 (cmake_minimum_required):
  Compatibility with CMake < 2.8.12 will be removed from a future version of
  CMake.

  Update the VERSION argument <min> value or use a ...<max> suffix to tell
  CMake that the project does not need compatibility with older versions.

And later the build fails with:

[ 36%] Building CXX object antlr/antlr4-cpp-runtime-4.7.2-source/runtime/CMakeFiles/antlr4_static.dir/src/ParserInterpreter.cpp.o
[ 36%] Building CXX object antlr/antlr4-cpp-runtime-4.7.2-source/runtime/CMakeFiles/antlr4_static.dir/src/ParserRuleContext.cpp.o
[ 36%] Building CXX object antlr/antlr4-cpp-runtime-4.7.2-source/runtime/CMakeFiles/antlr4_static.dir/src/ProxyErrorListener.cpp.o
[ 37%] Building CXX object antlr/antlr4-cpp-runtime-4.7.2-source/runtime/CMakeFiles/antlr4_static.dir/src/RecognitionException.cpp.o
[ 37%] Building CXX object antlr/antlr4-cpp-runtime-4.7.2-source/runtime/CMakeFiles/antlr4_static.dir/src/Recognizer.cpp.o
In file included from /home/bernd/build/Silice/007draft/Silice/antlr/antlr4-cpp-runtime-4.7.2-source/runtime/src/atn/PredictionContext.h:10,
                 from /home/bernd/build/Silice/007draft/Silice/antlr/antlr4-cpp-runtime-4.7.2-source/runtime/src/atn/ATNSimulator.h:11,
                 from /home/bernd/build/Silice/007draft/Silice/antlr/antlr4-cpp-runtime-4.7.2-source/runtime/src/Recognizer.cpp:12:
/home/bernd/build/Silice/007draft/Silice/antlr/antlr4-cpp-runtime-4.7.2-source/runtime/src/atn/ATNState.h:73:26: warning: type attributes ignored after type is already defined [-Wattributes]
   73 |   class ANTLR4CPP_PUBLIC ATN;
      |                          ^~~
[ 38%] Building CXX object antlr/antlr4-cpp-runtime-4.7.2-source/runtime/CMakeFiles/antlr4_static.dir/src/RuleContext.cpp.o
In file included from /home/bernd/build/Silice/007draft/Silice/antlr/antlr4-cpp-runtime-4.7.2-source/runtime/src/RuleContext.cpp:10:
/home/bernd/build/Silice/007draft/Silice/antlr/antlr4-cpp-runtime-4.7.2-source/runtime/src/atn/ATNState.h:73:26: warning: type attributes ignored after type is already defined [-Wattributes]
   73 |   class ANTLR4CPP_PUBLIC ATN;
      |                          ^~~
[ 38%] Building CXX object antlr/antlr4-cpp-runtime-4.7.2-source/runtime/CMakeFiles/antlr4_static.dir/src/RuleContextWithAltNum.cpp.o
cc1plus: all warnings being treated as errors
CMakeFiles/libsilice.dir/build.make:115: recipe for target 'CMakeFiles/libsilice.dir/src/LuaPreProcessor.cpp.o' failed
make[2]: *** [CMakeFiles/libsilice.dir/src/LuaPreProcessor.cpp.o] Error 1
CMakeFiles/Makefile2:169: recipe for target 'CMakeFiles/libsilice.dir/all' failed
make[1]: *** [CMakeFiles/libsilice.dir/all] Error 2
make[1]: *** Waiting for unfinished jobs....

Regards, Bernd.

@at91rm9200
Copy link

Hello Sylvain,

I used the CMakeLists.txt from the master branch and could compile the draft branch with it. Now I have to upgrade Python, since Edalize seems to need a newer version...

Regards, Bernd.

@sylefeb
Copy link
Owner

sylefeb commented Jan 19, 2024

Hi Bernd, thanks! I'll be looking into this shortly (this is due to the compiler treating warnings as errors, I thought I fixed it but clearly the change did not propagate).

@at91rm9200
Copy link

Hello Sylvain,

I am now using the new bit select "feature" to bind the inouts and my design is working. Thank you very much for implementing this.

May be there is still a little problem, since Silice throws a lot of warnings that "bindings have inconsistent bit-widths", although the bit length is clearly one bit. Shall I open a new issue for this?

Then I had to use the silice-make.py from the master branch. Otherwise I got the following error:

<<=- compiling blinky.si for icestick -=>>
using default variant  configurable
using variant          configurable
using build system     edalize
Traceback (most recent call last):
  File "/home/bernd/fpga/silice/2401draft_weg/Silice/bin/silice-make.py", line 274, in <module>
    from edalize.edatool import get_edatool
ImportError: cannot import name 'get_edatool'
Makefile:3: recipe for target 'icestick' failed
make: *** [icestick] Error 1

And at last, I had to use the "dynamic linked variant" of Silice, which is produced with the master version of CMakeLists.txt. Executing the "static variant" leads to a segmentation fault on my machine.

Regards, Bernd.

@sylefeb
Copy link
Owner

sylefeb commented Jan 20, 2024

Hi Bernd,

Thanks for the report and for having gone through all the trouble, I'm working on fixing all these issues asap!

Best,
Sylvain

@sylefeb
Copy link
Owner

sylefeb commented Jan 22, 2024

Hi Bernd,

All of these issues are fixed in the draft branch. Regarding compilation, I created a 'getting started' script ./get_started_linux.sh that I tested under Ubuntu (18, 20), Debian, Fedora, ArchLinux. Please note however that everything is now installed in the usual /usr/local/bin and /usr/local/share/silice directories, and the script also downloads and sets up an FPGA toolchain (oss-cad-suite, gets installed into /usr/local/share/silice/oss-cad-suite). It does add a line to .bashrc for setting up the environment. Of course you can still choose to compile Silice as before, only the installation paths are changed.

To avoid the old silice executable to be in the path it may be best to remove it from Silice/bin.

The warning is fixed too.

Hope that these changes will go smoothly!
Best,
Sylvain

@at91rm9200
Copy link

Hello Sylvain,

I compiled the old way via compile_silice_linux.sh. This went smooth. Thanks.

/* Rant about Python removed ;-) */

There are still warnings left. Silice code:

   uint8 io_oe(0); uint8 io_i(0); uint8 io_o(0);
   sb_io_inout sb_io0(clock <: clock, oe <: io_oe[0,1], in :> io_i[0,1], out <: io_o[0,1], pin <:> sdr_dq[0,1]);
[warning]    (/home/bernd/fpga/silice/2402draft/Silice/mue/project/test_sdram/sdram_8.si, line   67) 
             instance 'sb_io0', binding 'clock' to 'clock', bindings have inconsistent bit-widths

I can provide a full test case, if you need it.

Regards, Bernd.

@HusseinSam
Copy link

HusseinSam commented Jul 10, 2024

Hello sylefeb,

I'm using Linux and starting to use Silice. In my first step, I tried to test it as you instructed:

cd projects
cd divstd_bare
make icarus

However, it shows me an error that it can't recognize the 'silice' command." , should i do something else after i cloned the repository and run ./get_started_linux.sh ? , its does not clear for me.
regards, Hussein.

@sylefeb
Copy link
Owner

sylefeb commented Jul 10, 2024

Hello, did you run compile_silice_linux.sh ? It is responsible for compiling and installing Silice (see also GetStarted_Linux). As the compilation script does a make install, the script will request sudo access. (edit: note on sudo)

@HusseinSam
Copy link

HusseinSam commented Jul 10, 2024

no i did not run "compile_silice_linux.sh" , because its already included in ./get_started_linux.sh command as you can see in the picture :
Screenshot 2024-07-10 155031

and i got this output also on my terminal ( which is from compile_silice_linux.sh file ) :
so its ok ?
Screenshot 2024-07-10 155302

and another question should i do this by my self ?

Please compile and install:

  • yosys
  • trellis, icestorm, nextpnr
  • verilator
  • icarus verilog

@sylefeb
Copy link
Owner

sylefeb commented Jul 10, 2024

These last steps should no longer be necessary if using the get_started_linux.sh command as it installs pre-built binaries from oss-cad-suite (I'll update this message). Can you check the content of Silice/BUILD/build-silice? There should be a silice executable in there. Were there any errors reporting when executing the script? (feel free to attach the full console log).

@HusseinSam
Copy link

HusseinSam commented Jul 10, 2024

here is the output :
no error but there are many warnnings like this :
image

@HusseinSam
Copy link

HusseinSam commented Jul 10, 2024

now its ok and i have silice in my linux , but when i run the test following the steps :
as you instructed:

cd projects
cd divstd_bare
make icarus

this happened : (i did not get gtkwave window ) does this error look familiar to you or its just take time ?

image

the VGA test works 👍
image

@sylefeb
Copy link
Owner

sylefeb commented Jul 10, 2024

That's great news! The fact that divstd_bare does not stop is not normal, and I can confirm I see the same issue; I'll fix it asap. Thanks for trying out Silice and for reaching out with these reports!

@HusseinSam
Copy link

by the way , now I'm working on project , that takes a system Verilog baseline that implement a vga draw and try to implement it in Silice and compare performance.
do you have any tips from where to start ?
and a good environment to write Silice code and compile/debug the code ?
thank you in advance.

@sylefeb
Copy link
Owner

sylefeb commented Jul 11, 2024

On VGA there are multiple Silice examples, projects/vga_demo is a good starting point, in particular vga_rototexture and vga_flyover3d (other examples are more advanced, with pipelines). See also the simple projects/vga_test, even though this one as a lot of configuration for testing various boards (the lines with $$ are preprocessor directives).

In terms of performance, are you looking at cycle counts, LUT count, max frequency? It is normally possible to reach the exact same result as Verilog, depending on which blend you choose between using alway blocks/algorithms/pipelines. Of course using the more flexible syntax can yield to overheads (but not always).

For coding environment, I use vscode and there is a syntax coloring theme in tools\vscode. It has to be manually installed for now (simply copying the files in the vscode extensions folder).

@sylefeb
Copy link
Owner

sylefeb commented Jul 12, 2024

New: the vscode extension is now part of the vscode library, and can be found in the Extensions tab of vscode (search for 'silice').

@HusseinSam
Copy link

Thank you for your help in setting this up, Everything is working perfectly now. Your assistance was greatly appreciated!

@HusseinSam
Copy link

Hello [sylefeb],

How are you doing? I have a couple of questions about Silice code.

The first one: If I'm writing a VGA demo in Silice and I'm interested in the generated SystemVerilog code, I notice that the build.v file automatically adds prefixes to the parameters like this:

image
Is it possible to remove the prefix, or do I have to do it manually?

The second question is: How can I compile a simple Silice code without using another tool like Verilator? Sometimes I write simple Silice code that is not a VGA demo, and I just want it to generate SystemVerilog code( i use the generated code for a project). How can I do this?
for example this code :
image

The last question: I saw that the generated SystemVerilog code from Silice includes a reset by default. If I want to include a reset input in my code, how can I write it in Silice?

In the code bellow, there's a SystemVerilog code that has reset as an input:
image

However, when I try to add a reset input in Silice, I end up with two reset inputs in the generated sv code , as we can see in the first picture (in_rst and reset). How can I avoid this duplication?

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

No branches or pull requests

4 participants