Two points that I would like to make:
* The opening comment in the patch states that it is trying to do two
things.  I would suggest that we split the patch.

* I think we should not drop the original behaviour.  Firstly, it was not incorrect.
Secondly, no reason has been provided as to why the behaviour implemented
should be preferred.  Are we sure that most out-of-order processors would
choose the proposed over the original?

I'll take Mitch and Stephen's word that we need this patch.  I'll still 
like to understand it in more detail before we commit it.

src/cpu/o3/inst_queue.hh (Diff revision 1)

I think we need better differentiation between this list and the one declared after it.

On further reading, it seems that we may not need the two lists.  Can we just mark the instructions that they should be retried?  While adding them back to the ready queue, we can check which ones are marked.  Or may be keep an iterator that tracks the point till which we 
should retry.

One more thought,  can we do with a queue instead of a list?

Show all issues

1. Mitch Hayenga Aug. 19, 2014, 12:41 p.m. (Aug. 19, 2014, 12:41 p.m.)

   That could be done.  Real hardware would likely mark these "blocked" instructions with a bit to guard from execution in the IQ.  The clearing of the blocked cause (cache blocked in this case) would flash-clear that guard bit for all load instructions in the IQ.
   
   Functionally these two approaches should be equivalent.  The same alternative implementation could have been done for "deferred" memory instructions.  I just coded it similarly to deferred instructions were already handled.  I haven't looked to verify how easy this alternative implementation would be to do.
   
   A queue could be used, the only difference would be that the clear() would be replaced with a swap with an empty queue.

src/cpu/o3/inst_queue_impl.hh (Diff revision 1)

Should we not clear retryMemInsts as well?

Show all issues

1. Mitch Hayenga Aug. 19, 2014, 12:41 p.m. (Aug. 19, 2014, 12:41 p.m.)

   Yep, will add.

src/cpu/o3/inst_queue_impl.hh (Diff revision 1)

Let's retain the new line above this while loop.

1. Mitch Hayenga Aug. 19, 2014, 12:41 p.m. (Aug. 19, 2014, 12:41 p.m.)

   Ok.

src/cpu/o3/inst_queue_impl.hh (Diff revision 1)

We should use nullptr now that we have gcc minimum dependency at 4.6.

1. Mitch Hayenga Aug. 19, 2014, 12:41 p.m. (Aug. 19, 2014, 12:41 p.m.)

   Hmm, at one point in this patch or another I had to try to return nullptr and someone took issue with it I believe.  Either way, personally I don't care.
   
   Grepping our code base (this might be different from the mainline gem5) it seems we only use nullptr eleven times in the code base and only in network-related code.  Should a later "consistency" patch try to change this?  Because currently it looks like NULL is the preferred.
   
   $ grep -r NULL  * | wc -l
       1515
   $ grep -r nullptr  * | wc -l
         11

2. Nilay Vaish Aug. 23, 2014, 9:10 a.m. (Aug. 23, 2014, 9:10 a.m.)

   We can have a single patch or we can do it in a lazy fashion.  I am fine with either.
   Any new code that goes in should use nullptr.  The incident you mention probably
   happened before we moved to GCC 4.6 being the minimum required version.

src/cpu/o3/lsq_unit.hh (Diff revision 1)

Do we need all these changes that appear over next 15-20 lines?  It seems from my initial reading that the previous code structure could have been retained.

Show all issues

1. Mitch Hayenga Aug. 19, 2014, 12:41 p.m. (Aug. 19, 2014, 12:41 p.m.)

   I moved it below during the coding to clearly show the deletion events.  I hit a few issues due to bad deletions and split loads.  Having all deletions done together and not mixed in with code segments above made it clearer to me.

src/cpu/o3/lsq_unit_impl.hh (Diff revision 1)

New line after.

1. Mitch Hayenga Aug. 19, 2014, 12:41 p.m. (Aug. 19, 2014, 12:41 p.m.)

   Will do.