src/arch/x86/insts/microregop.cc (Diff revision 1)

I don't think this should be split out into a separate function.

Show all issues

1. Nilay Vaish April 28, 2012, 11:37 p.m. (April 28, 2012, 11:37 p.m.)

   the reason being?

2. Gabe Black April 29, 2012, 9:16 a.m. (April 29, 2012, 9:16 a.m.)

   Sorry, I should have explained. Lets say we have 6 flag registers like I think we're going to, and for simplicity I'll call the A, B, C, D, E, and F. If we have a separate function for each, that will look something like the following:
   
   void setA(...)
   {
   ...
   }
   
   void setB(...)
   {
   ...
   }
   
   etc.
   
   setA()
   serB()
   setC()
   setD()
   setE()
   setF()
   
   compare that to still having a single function which sets all the registers together. In one case we have six calls to six small functions, and in the other we have one call to a larger function. Since the pieces of the larger function make sense to put together (as opposed to random stuff being shoved into a single function), the primary effects are that we save the overhead of 5 function calls, and we can avoid some of the redundancies we'd get from the smaller functions like ECF and EZF being essentially the same as the code for CF and ZF. It's possible the compiler would figure out that those smaller functions could be inlined into a larger function behind the scenes anyway, but I think the code is cleaner and simpler if we keep things together.

3. Nilay Vaish May 1, 2012, 11:58 a.m. (May 1, 2012, 11:58 a.m.)

   I had thought of keeping a single function, but it seems a couple of things go 
   against this choice. Firstly, the number of arguments will go up. Secondly, 
   currently we return the newly computed value. If we update all the registers 
   in a single function, then we will have to pass these by reference. I don't 
   know if the ISA parser will still recognise that these registers are being
   written on to.

Should I assume that no one is opposed to this patch in its current form? This
would clear my way in further breaking up the condition code register.

This seems to be basically going in the right direction, but there are a lot of rough edges to smooth out.

src/arch/x86/isa/microops/debug.isa (Diff revision 3)

There should be spaces around |, here and below.

Show all issues

src/arch/x86/isa/microops/regop.isa (Diff revision 3)

What are these constants? If this is splitting out the fields you mention in the commit message, that's already basically done. ccFlagBits means condition code flag bits. At the very least these constants should have names. Better yet, they should be derived from constants that already exist (ECFBit, EZFBit).

Show all issues

1. Nilay Vaish May 18, 2012, 4:59 a.m. (May 18, 2012, 4:59 a.m.)

   We need to figure out the individual register values after they have been
   updated by the genFlags() function.

src/arch/x86/isa/microops/regop.isa (Diff revision 3)

Don't put in a bunch of white space to make things line up. Tables are for data, not code. Unless you have a lot of data lining that up isn't all that useful either.

Show all issues

src/arch/x86/isa/microops/regop.isa (Diff revision 3)

It looks like either you forgot to set the emulation flags here or you forgot to put this back on one line. Regardless, I think you should set EZF and ECF here to stay consistent with the old semantics. Things will need to be that way anyway once we have a bunch of registers.

Show all issues

src/arch/x86/isa/microops/regop.isa (Diff revision 3)

Why the change in alignment?

Show all issues

src/arch/x86/isa/operands.isa (Diff revision 3)

This name isn't great. At least leave Flag in there so I know what sort of bits they are (ucc by itself signifies almost nothing).

Show all issues

A solid step in the right direction. There are still some issues to address.

The most important one is that you don't want to compute a value, put it in an operand the parser recognizes, and then use that operand to compute other values. You're not using the value the operand used to have, but the parser isn't smart enough to know that and assumes it must be a source as well. You'll find a lot of places in the various ISA descriptions where things are put in a temporary variable, assigned to the destination, and then the temporary is used to compute flags.

The other less serious style issues should still be addressed though, and be sure to test carefully.

src/arch/x86/isa/microops/fpop.isa (Diff revision 5)

This alignment is still not necessary.

Show all issues

src/arch/x86/isa/microops/regop.isa (Diff revision 5)

This will unintentionally make the parser think ccFlagBits is a source. You'll want to use a temporary for psrc1 ^ op2. I think this problem crops up in a lot of this patch.

Show all issues

src/arch/x86/isa/microops/regop.isa (Diff revision 5)

} on the same line as else

Show all issues

Address my existing feedback before you post new code (unless this was already written).

You have more than it looked like you had the other day. I blame stale browser cache. In any case, you really did miss a couple spots.

src/arch/x86/isa/microops/mediaop.isa (Diff revision 6)

Missed one.

Show all issues

src/arch/x86/isa/microops/regop.isa (Diff revision 6)

Missed one.

Show all issues

Great, thanks for the fixes. I think I'm happy with this.