Simulator: base_insn and insn in decode.c

Simulator: base_insn and insn in decode.c

[Ronald Hecht]

Hello again,

i found another problem in decode. c. As I have variable instruction 
size, decode.c does not handle base_insn and insn in the right way. The 
file looks like this:

const IDESC *
proc8bf_decode (SIM_CPU *current_cpu, IADDR pc,
              CGEN_INSN_INT base_insn, CGEN_INSN_INT entire_insn,
              ARGBUF *abuf)
{
  /* Result of decoder.  */
  PROC8BF_INSN_TYPE itype;

  {
    CGEN_INSN_INT insn = base_insn;

    {
      unsigned int val = (((insn >> 0) & (63 << 0)));
      switch (val)
      {
      case 0 :
        if ((entire_insn & 0xff) == 0x0)
          { itype = PROC8BF_INSN_NOP; goto extract_sfmt_nop; }
        itype = PROC8BF_INSN_X_INVALID; goto extract_sfmt_empty;
      case 1 :
        if ((entire_insn & 0xff) == 0x1)
          { itype = PROC8BF_INSN_LDA; goto extract_sfmt_lda; }
        itype = PROC8BF_INSN_X_INVALID; goto extract_sfmt_empty;
      case 2 :
        if ((entire_insn & 0xff) == 0x2)
          { itype = PROC8BF_INSN_LDC; goto extract_sfmt_ldc; }
        itype = PROC8BF_INSN_X_INVALID; goto extract_sfmt_empty;
 
<snip>

I have given the first instruction byte as base_insn and entire_insn is 
the complete instruction. The location of the base instruction depends 
on the instruction size. So the calculation

      unsigned int val = (((insn >> 0) & (63 << 0)));

is not correct. If I remove the if-statement around the goto 
extract_sfmt_<insn> everything works correct. The simulatior does the 
right things.

By the way: I found it a bit confusing to write mloop.in. It was 
difficult to handle different size instructions. I did it this way:

xextract-pbb)

# Inputs:  current_cpu, pc, sc, max_insns, FAST_P
# Outputs: sc, pc
# sc must be left pointing past the last created entry.
# pc must be left pointing past the last created entry.
# If the pbb is terminated by a cti insn, SET_CTI_VPC(sc) must be called
# to record the vpc of the cti insn.
# SET_INSN_COUNT(n) must be called to record number of real insns.

cat <<EOF
{
  const IDESC *idesc;
  int icount = 0;

  while (max_insns > 0)
    {
      CGEN_INSN_INT insn;
      CGEN_INSN_INT base_insn = GETIMEMUQI (current_cpu, pc);
      idesc = @cpu@_decode(current_cpu, pc, base_insn, base_insn, 
&sc->argbuf);
      switch (idesc->length) {
      case 1 : break;
      case 2 :
    insn = base_insn * 256 + GETIMEMUQI (current_cpu, pc + 1);
    break;
      case 3 :
    insn = base_insn * 256 * 256 + GETIMEMUQI (current_cpu, pc + 1) * 
256 + GETIMEMUQI (current_cpu, pc + 2);
    break;
      }
      idesc = @cpu@_decode(current_cpu, pc, base_insn, insn, &sc->argbuf);
      @cpu@_fill_argbuf (current_cpu, &sc->argbuf, idesc, pc, FAST_P);
      if (! FAST_P) {
    int trace_p = PC_IN_TRACE_RANGE_P (current_cpu, pc);
    int profile_p = PC_IN_PROFILE_RANGE_P (current_cpu, pc);
    @cpu@_fill_argbuf_tp (current_cpu, &sc->argbuf, trace_p, profile_p);
      }
      ++sc;
      --max_insns;
      ++icount;
      pc += idesc->length;
      if (IDESC_CTI_P (idesc))
    {
      SET_CTI_VPC (sc - 1);
      break;
    }
    }
 
 Finish:
  SET_INSN_COUNT (icount);
}
EOF

;;

Is there an easier way to write this down?

Ronald

Re: Simulator: base_insn and insn in decode.c

[Dave Brolley]

Ronald Hecht wrote:

> Hello again,
>
> i found another problem in decode. c. As I have variable instruction 
> size, decode.c does not handle base_insn and insn in the right way. 
> The file looks like this:
>
> const IDESC *
> proc8bf_decode (SIM_CPU *current_cpu, IADDR pc,
>              CGEN_INSN_INT base_insn, CGEN_INSN_INT entire_insn,
>              ARGBUF *abuf)
> {
>  /* Result of decoder.  */
>  PROC8BF_INSN_TYPE itype;
>
>  {
>    CGEN_INSN_INT insn = base_insn;
>
>    {
>      unsigned int val = (((insn >> 0) & (63 << 0)));
>      switch (val)
>      {
>      case 0 :
>        if ((entire_insn & 0xff) == 0x0)
>          { itype = PROC8BF_INSN_NOP; goto extract_sfmt_nop; }
>        itype = PROC8BF_INSN_X_INVALID; goto extract_sfmt_empty;

The generated decoder expects base_insn to be the first base-insn-size 
bits of the insn shifted as far right as possible. What you have 
specified is correct and the (((insn >> 0) & (63 << 0))) test us 
correct. It is looking at the low order 6 bits of the base_insn which 
are the only significant ones (according to your cpu file.

The generated decoder expects entire_insn to the all of the insn bits, 
also shifted as far right as possible.

The test around the goto is intended to make sure that the untested 
base_insn bits are as expected. It needs to be there, otherwise, invalid 
insns get recognized as valid ones. However, it would seem that the test 
should be against base_insn and not against entire_insn. I'll have a 
look at some other cgen simulators, including those which use SID to 
make sure that this assertion holds water. It probably hasn't come up 
until now, since most sims I have seen are able to pass the same value 
for base_insn and entire_insn.

Dave

Re: Simulator: base_insn and insn in decode.c

[Dave Brolley]

I forgot to address your other question which was about the difficulty 
of setting entire_insn before decoding. It's a bit of a chicken and egg 
situation isnt'y it? You can't set entire insn properly until you know 
something about the insn, but you can't call decode without setting 
entire_insn properly.

Your method of calling decode twice won't work because of this.

Most existing ports determine the insn length by looking at a few bits 
in base_insn and use this information to fill entire_insn.

I'm wondering if the decoder shouldn't be redesigned so that all it 
takes is base_insn, which should be all that is necessary to identify 
the insn. The extractors for each format would then read any additional 
bytes as needed. There is already support for this for ISAs with insns 
which are longer than an insn word.

Dave

Re: Simulator: base_insn and insn in decode.c

[Ronald Hecht]

Dave Brolley wrote:

> Ronald Hecht wrote:
>
>> Hello again,
>>
>> i found another problem in decode. c. As I have variable instruction 
>> size, decode.c does not handle base_insn and insn in the right way. 
>> The file looks like this:
>>
>> const IDESC *
>> proc8bf_decode (SIM_CPU *current_cpu, IADDR pc,
>>              CGEN_INSN_INT base_insn, CGEN_INSN_INT entire_insn,
>>              ARGBUF *abuf)
>> {
>>  /* Result of decoder.  */
>>  PROC8BF_INSN_TYPE itype;
>>
>>  {
>>    CGEN_INSN_INT insn = base_insn;
>>
>>    {
>>      unsigned int val = (((insn >> 0) & (63 << 0)));
>>      switch (val)
>>      {
>>      case 0 :
>>        if ((entire_insn & 0xff) == 0x0)
>>          { itype = PROC8BF_INSN_NOP; goto extract_sfmt_nop; }
>>        itype = PROC8BF_INSN_X_INVALID; goto extract_sfmt_empty;
>
>
> The generated decoder expects base_insn to be the first base-insn-size 
> bits of the insn shifted as far right as possible. What you have 
> specified is correct and the (((insn >> 0) & (63 << 0))) test us 
> correct. It is looking at the low order 6 bits of the base_insn which 
> are the only significant ones (according to your cpu file.


I completely agree with that. This actually works.

>
> The generated decoder expects entire_insn to the all of the insn bits, 
> also shifted as far right as possible.


Yes, but im my case entire instruction starts at bit 23 with the 
instruction opcode. Bit 15 is the start of an 16 Bit operand. In the 
case of a 16 Bit instruction the opcode starts at bit 15 and then comes 
a 8 bit immediate. The decode functionality beneath decodes this correctly:

<snip>
 extract_sfmt_lda:
  {
    const IDESC *idesc = &proc8bf_insn_data[itype];
    CGEN_INSN_INT insn = entire_insn;
#define FLD(f) abuf->fields.sfmt_lda.f
    UINT f_uimm16;

    f_uimm16 = EXTRACT_MSB0_UINT (insn, 24, 8, 16);

  /* Record the fields for the semantic handler.  */
  FLD (f_uimm16) = f_uimm16;
</snip>

or

<snip>
 extract_sfmt_ldc:
  {
    const IDESC *idesc = &proc8bf_insn_data[itype];
    CGEN_INSN_INT insn = entire_insn;
#define FLD(f) abuf->fields.sfmt_ldc.f
    UINT f_uimm8;

    f_uimm8 = EXTRACT_MSB0_UINT (insn, 16, 8, 8);

  /* Record the fields for the semantic handler.  */
  FLD (f_uimm8) = f_uimm8;
</snip>

So this is correct, but the if clauses in the case statement should use 
the correct bits to double-check. As I mentioned, when I remove the if 
clauses, everything works fine. The simulator works as expected.

>
> The test around the goto is intended to make sure that the untested 
> base_insn bits are as expected. It needs to be there, otherwise, 
> invalid insns get recognized as valid ones. However, it would seem 
> that the test should be against base_insn and not against entire_insn. 
> I'll have a look at some other cgen simulators, including those which 
> use SID to make sure that this assertion holds water. It probably 
> hasn't come up until now, since most sims I have seen are able to pass 
> the same value for base_insn and entire_insn.
>
> Dave
>

I agreee. The test should be against base_insn. But then it still seems 
to be double-checking against the same, or not?

By the way, I'm preparing a website holding the stuff about the 
processor and the tools port. It was pretty difficult to find some 
useful information about how to port the GNU tools starting from a very 
simple example. So I decided to put this information on this website.

http://www-md.e-technik.uni-rostock.de/lehre/vlsi_i/proc8/

Ronald

Re: Simulator: base_insn and insn in decode.c

[Ronald Hecht]

Dave Brolley wrote:

> I forgot to address your other question which was about the difficulty 
> of setting entire_insn before decoding. It's a bit of a chicken and 
> egg situation isnt'y it? You can't set entire insn properly until you 
> know something about the insn, but you can't call decode without 
> setting entire_insn properly.



I agree, that's always the case.

>
> Your method of calling decode twice won't work because of this.
>
> Most existing ports determine the insn length by looking at a few bits 
> in base_insn and use this information to fill entire_insn.
>
> I'm wondering if the decoder shouldn't be redesigned so that all it 
> takes is base_insn, which should be all that is necessary to identify 
> the insn. The extractors for each format would then read any 
> additional bytes as needed. There is already support for this for ISAs 
> with insns which are longer than an insn word.



That sounds good. Actually, I'm wondering a bit why the disassembler in 
src/opcodes works so great and the simulator port only under certain 
circumstances.  Anyway. CGEN is great stuff. It helps a lot. Whould you 
say that SID is a better choice? Does it work with gdb as SIM does?

Ronald

Re: Simulator: base_insn and insn in decode.c

[Ronald Hecht]

I switched to the SID framework. I found it easier to create a new 
target. The disassemby works better as it calls this disassembler code 
in src/opcodes. But I'm still in trouble with decode.cxx. It looks the 
same as in sim. I'n my opinion the entire_insn should alwasy be left 
aligned and not right_aligned as it is now. Base_insn should have the 
same alignment. I think this way it is easier to support variable sized 
instructions.

Best Regards
Ronald

Dave Brolley wrote:

> I forgot to address your other question which was about the difficulty 
> of setting entire_insn before decoding. It's a bit of a chicken and 
> egg situation isnt'y it? You can't set entire insn properly until you 
> know something about the insn, but you can't call decode without 
> setting entire_insn properly.
>
> Your method of calling decode twice won't work because of this.
>
> Most existing ports determine the insn length by looking at a few bits 
> in base_insn and use this information to fill entire_insn.
>
> I'm wondering if the decoder shouldn't be redesigned so that all it 
> takes is base_insn, which should be all that is necessary to identify 
> the insn. The extractors for each format would then read any 
> additional bytes as needed. There is already support for this for ISAs 
> with insns which are longer than an insn word.
>
> Dave
>

Re: Simulator: base_insn and insn in decode.c

[Dave Brolley]

I've been on vacation....

Ronald Hecht wrote:

>> The test around the goto is intended to make sure that the untested 
>> base_insn bits are as expected. It needs to be there, otherwise, 
>> invalid insns get recognized as valid ones. However, it would seem 
>> that the test should be against base_insn and not against 
>> entire_insn. I'll have a look at some other cgen simulators, 
>> including those which use SID to make sure that this assertion holds 
>> water. It probably hasn't come up until now, since most sims I have 
>> seen are able to pass the same value for base_insn and entire_insn.
>>
>> Dave
>>
>
>
> I agreee. The test should be against base_insn. But then it still 
> seems to be double-checking against the same, or not?

No it's not a redundant test. The decoder only tests enough bits in the 
switch to uniquely separate the valid insns. There may be invalid bit 
patterns (i.e. unused opcodes) that still match. The secondary test 
makes sure that *all* of the opcode bits in the insn are tested.

Dave

Re: Simulator: base_insn and insn in decode.c

[Dave Brolley]

Ronald Hecht wrote:

> I switched to the SID framework. I found it easier to create a new 
> target. The disassemby works better as it calls this disassembler code 
> in src/opcodes. But I'm still in trouble with decode.cxx. It looks the 
> same as in sim. I'n my opinion the entire_insn should alwasy be left 
> aligned and not right_aligned as it is now. Base_insn should have the 
> same alignment. I think this way it is easier to support variable 
> sized instructions.
>
I've been looking at what other ports do and it seems that most are able 
to pass the same value base_insn and entire_insn because the opcode bits 
are scattered throughout the insns. The ones which aren't so lucky do 
one of two things:

1) Write some ugly code which examines the base_insn bits in order to 
decide how to position entire_insn.

2) Set base-insn-bitsize in the define-isa of the .cpu file to be the 
size of the largest insn. For SID, base_insn and entire_insn can then be 
passed identically as the insn bits aligned at the 'base-insn-bitsize' 
bit. For example, in your case, the max insn length appears to be 24, so 
you could simply read 3 bytes individually (to avoid endianness 
problems) and place them sequentially in the low order bytes of 
base_insn and entire_insn.

Barring some redesign of the interface as has been discussed, I don't 
see any other options for you.

Dave

Re: Simulator: base_insn and insn in decode.c

[Ronald Hecht]

Dave Brolley wrote:

> I've been looking at what other ports do and it seems that most are 
> able to pass the same value base_insn and entire_insn because the 
> opcode bits are scattered throughout the insns. The ones which aren't 
> so lucky do one of two things:
>
> 1) Write some ugly code which examines the base_insn bits in order to 
> decide how to position entire_insn.


This sounds good, but it would be helpful to have the instruction size 
in the automatically generated instruction descriptor table. I say this 
in the disassembler code. This way a lookup would be easier. The 
instruction size may be also used in the <cpu>-sem.cxx code.

>
> 2) Set base-insn-bitsize in the define-isa of the .cpu file to be the 
> size of the largest insn. For SID, base_insn and entire_insn can then 
> be passed identically as the insn bits aligned at the 
> 'base-insn-bitsize' bit. For example, in your case, the max insn 
> length appears to be 24, so you could simply read 3 bytes individually 
> (to avoid endianness problems) and place them sequentially in the low 
> order bytes of base_insn and entire_insn.
>

This actually works for me! Thanks alot for the help. Now, my code looks 
like this:

      try
        {
          // Fetch 24-bit pieces separately, so endianness
          // conversions can be done on this chunk size.
          UQI insn0 = this->GETIMEMQI (pc, pc);
          UQI insn1 = this->GETIMEMQI (pc, pc+1);
          UQI insn2 = this->GETIMEMQI (pc, pc+2);
          USI insn = (insn0 << 16) | (insn1 << 8) | insn2;

          sem->decode (this, pc, insn, insn);

        }
      catch (cpu_memory_fault& t)
        {
          this->memory_trap (t);
          break;
        }

The file can be found on 
http://www-md.e-technik.uni-rostock.de/lehre/vlsi_i/proc8/src/sid/component/cgen-cpu/proc8/proc8bf.cxx
The simulator also works with gdb except for a "Memory access error 
while loading section .text." occuring some times. But I suspect, this 
is out of the scope of this mailing list.

Thanks again
Ronald