Re: [gem5-dev] Review Request: inorder/dtb: make sure DTB translate correct address

2011-06-12 Thread Korey Sewell


 On 2011-06-11 09:56:39, Steve Reinhardt wrote:
  Shouldn't you get rid of the cache_unit.cc changes now?  I thought that was 
  the point.
  
  This is still a hack, in my opinion; note that the comment on the _pc field 
  in mem/request.hh says for tracing/debugging, i.e., it's not intended to 
  be architectural.  Also, it isn't always set (e.g., for device accesses), 
  though for CPU accesses it should be.  So I'd say (1) it should work and 
  (2) it's a much less ugly hack than what you had before, so assuming you do 
  get rid of the cache_unit.cc changes I'd say it's fine.
  
  I still think having a ProxyThreadContext wrapped around a DynInst is the 
  right way to do it, but I can see where that also looks like a lot of 
  mostly unnecessary overhead.

The hacky cache_unit.cc code will definitely be cleaned up in the final change. 

I was thinking that although the ProxyThreadContextDynInst seems like the 
right way to do this for DTB accesses, I think it may not work well for ITB 
accesses as it could be possible that the DynInst has not even been created 
yet. Typically, one might fetch the cache block and then create the DynInst 
(done in simple and o3) rather than vice versa (done in inorder).

I'm thinking the right thing to do is to make the _pc field in Request more 
than a debugging feature, and then add a isSet flag there along with an  
assertion in getPC() to make sure that any reader of the PC will not get a 
uninitialized value. That way, you have one generalized way to access the TLB 
whether it be ITB or DTB access.


- Korey


---
This is an automatically generated e-mail. To reply, visit:
http://reviews.m5sim.org/r/743/#review1323
---


On 2011-06-10 22:52:04, Korey Sewell wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://reviews.m5sim.org/r/743/
 ---
 
 (Updated 2011-06-10 22:52:04)
 
 
 Review request for Default, Ali Saidi, Gabe Black, Steve Reinhardt, and 
 Nathan Binkert.
 
 
 Summary
 ---
 
 inorder/dtb: make sure DTB translate correct address
 The DTB expects the correct PC in the ThreadContext
 but how if the memory accesses are speculative? Shouldn't
 we send along the requestor's PC to the translate functions?
 
 
 Diffs
 -
 
   src/arch/alpha/tlb.cc 77d12d8f7971 
   src/cpu/inorder/resources/cache_unit.cc 77d12d8f7971 
 
 Diff: http://reviews.m5sim.org/r/743/diff
 
 
 Testing
 ---
 
 
 Thanks,
 
 Korey
 


___
gem5-dev mailing list
gem5-dev@m5sim.org
http://m5sim.org/mailman/listinfo/gem5-dev


Re: [gem5-dev] Review Request: inorder/dtb: make sure DTB translate correct address

2011-06-11 Thread Steve Reinhardt

---
This is an automatically generated e-mail. To reply, visit:
http://reviews.m5sim.org/r/743/#review1323
---


Shouldn't you get rid of the cache_unit.cc changes now?  I thought that was the 
point.

This is still a hack, in my opinion; note that the comment on the _pc field in 
mem/request.hh says for tracing/debugging, i.e., it's not intended to be 
architectural.  Also, it isn't always set (e.g., for device accesses), though 
for CPU accesses it should be.  So I'd say (1) it should work and (2) it's a 
much less ugly hack than what you had before, so assuming you do get rid of the 
cache_unit.cc changes I'd say it's fine.

I still think having a ProxyThreadContext wrapped around a DynInst is the 
right way to do it, but I can see where that also looks like a lot of mostly 
unnecessary overhead.

- Steve


On 2011-06-10 22:52:04, Korey Sewell wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://reviews.m5sim.org/r/743/
 ---
 
 (Updated 2011-06-10 22:52:04)
 
 
 Review request for Default, Ali Saidi, Gabe Black, Steve Reinhardt, and 
 Nathan Binkert.
 
 
 Summary
 ---
 
 inorder/dtb: make sure DTB translate correct address
 The DTB expects the correct PC in the ThreadContext
 but how if the memory accesses are speculative? Shouldn't
 we send along the requestor's PC to the translate functions?
 
 
 Diffs
 -
 
   src/arch/alpha/tlb.cc 77d12d8f7971 
   src/cpu/inorder/resources/cache_unit.cc 77d12d8f7971 
 
 Diff: http://reviews.m5sim.org/r/743/diff
 
 
 Testing
 ---
 
 
 Thanks,
 
 Korey
 


___
gem5-dev mailing list
gem5-dev@m5sim.org
http://m5sim.org/mailman/listinfo/gem5-dev


Re: [gem5-dev] Review Request: inorder/dtb: make sure DTB translate correct address

2011-06-10 Thread Korey Sewell

---
This is an automatically generated e-mail. To reply, visit:
http://reviews.m5sim.org/r/743/
---

(Updated 2011-06-10 22:52:04.462095)


Review request for Default, Ali Saidi, Gabe Black, Steve Reinhardt, and Nathan 
Binkert.


Summary
---

inorder/dtb: make sure DTB translate correct address
The DTB expects the correct PC in the ThreadContext
but how if the memory accesses are speculative? Shouldn't
we send along the requestor's PC to the translate functions?


Diffs (updated)
-

  src/arch/alpha/tlb.cc 77d12d8f7971 
  src/cpu/inorder/resources/cache_unit.cc 77d12d8f7971 

Diff: http://reviews.m5sim.org/r/743/diff


Testing
---


Thanks,

Korey

___
gem5-dev mailing list
gem5-dev@m5sim.org
http://m5sim.org/mailman/listinfo/gem5-dev


Re: [gem5-dev] Review Request: inorder/dtb: make sure DTB translate correct address

2011-06-10 Thread Korey Sewell

---
This is an automatically generated e-mail. To reply, visit:
http://reviews.m5sim.org/r/743/#review1321
---



src/arch/alpha/tlb.cc
http://reviews.m5sim.org/r/743/#comment1767

The Request object has a PC in it which is used by the translateInst() 
function. 

Assuming that the PC object is always set correctly in Request, then 
translateData() can also use the Request to get the current PC value rather 
than the last committed state from ThreadContext.


- Korey


On 2011-06-10 22:52:04, Korey Sewell wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://reviews.m5sim.org/r/743/
 ---
 
 (Updated 2011-06-10 22:52:04)
 
 
 Review request for Default, Ali Saidi, Gabe Black, Steve Reinhardt, and 
 Nathan Binkert.
 
 
 Summary
 ---
 
 inorder/dtb: make sure DTB translate correct address
 The DTB expects the correct PC in the ThreadContext
 but how if the memory accesses are speculative? Shouldn't
 we send along the requestor's PC to the translate functions?
 
 
 Diffs
 -
 
   src/arch/alpha/tlb.cc 77d12d8f7971 
   src/cpu/inorder/resources/cache_unit.cc 77d12d8f7971 
 
 Diff: http://reviews.m5sim.org/r/743/diff
 
 
 Testing
 ---
 
 
 Thanks,
 
 Korey
 


___
gem5-dev mailing list
gem5-dev@m5sim.org
http://m5sim.org/mailman/listinfo/gem5-dev


Re: [gem5-dev] Review Request: inorder/dtb: make sure DTB translate correct address

2011-06-09 Thread Gabe Black

---
This is an automatically generated e-mail. To reply, visit:
http://reviews.m5sim.org/r/743/#review1311
---


This isn't a review, just a thought on the question you're asking. If the 
access is speculative, is it ok to use a misspeculated pc since the instruction 
will be thrown out anyway?

- Gabe


On 2011-06-08 23:34:50, Korey Sewell wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://reviews.m5sim.org/r/743/
 ---
 
 (Updated 2011-06-08 23:34:50)
 
 
 Review request for Default, Ali Saidi, Gabe Black, Steve Reinhardt, and 
 Nathan Binkert.
 
 
 Summary
 ---
 
 inorder/dtb: make sure DTB translate correct address
 The DTB expects the correct PC in the ThreadContext
 but how if the memory accesses are speculative? Shouldn't
 we send along the requestor's PC to the translate functions?
 
 
 Diffs
 -
 
   src/cpu/inorder/resources/cache_unit.cc 77d12d8f7971 
 
 Diff: http://reviews.m5sim.org/r/743/diff
 
 
 Testing
 ---
 
 
 Thanks,
 
 Korey
 


___
gem5-dev mailing list
gem5-dev@m5sim.org
http://m5sim.org/mailman/listinfo/gem5-dev


Re: [gem5-dev] Review Request: inorder/dtb: make sure DTB translate correct address

2011-06-09 Thread Steve Reinhardt


 On 2011-06-09 11:17:24, Gabe Black wrote:
  This isn't a review, just a thought on the question you're asking. If the 
  access is speculative, is it ok to use a misspeculated pc since the 
  instruction will be thrown out anyway?

Actually after briefly looking at the code, I wonder if we don't have a bigger 
problem here.  I'm assuming that the ThreadContext struct reflects the 
committed state of the machine, where we really want the state relative to this 
in-flight instruction.  Have we just been getting lucky so far?  Seems like in 
Alpha the TLB only uses the PC to see if it's in PAL mode or not, so maybe the 
pipeline gets flushed when we switch in and out of PAL mode and thus there's 
never an inconsistency there even in O3.  (Nate or Ali, can you confirm this?)

Korey, can you elaborate on the kind of errors you were running into without 
this change?

Technically I think perhaps the TLB should be using the ExecContext interface 
instead of the ThreadContext, but since ExecContext isn't a real object, that 
only works if you template the call... I wonder if we really should be using 
something like the ProxyThreadContext (see cpu/thread_context.hh) wrapped 
around a DynInst.  Does that make sense?


- Steve


---
This is an automatically generated e-mail. To reply, visit:
http://reviews.m5sim.org/r/743/#review1311
---


On 2011-06-08 23:34:50, Korey Sewell wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://reviews.m5sim.org/r/743/
 ---
 
 (Updated 2011-06-08 23:34:50)
 
 
 Review request for Default, Ali Saidi, Gabe Black, Steve Reinhardt, and 
 Nathan Binkert.
 
 
 Summary
 ---
 
 inorder/dtb: make sure DTB translate correct address
 The DTB expects the correct PC in the ThreadContext
 but how if the memory accesses are speculative? Shouldn't
 we send along the requestor's PC to the translate functions?
 
 
 Diffs
 -
 
   src/cpu/inorder/resources/cache_unit.cc 77d12d8f7971 
 
 Diff: http://reviews.m5sim.org/r/743/diff
 
 
 Testing
 ---
 
 
 Thanks,
 
 Korey
 


___
gem5-dev mailing list
gem5-dev@m5sim.org
http://m5sim.org/mailman/listinfo/gem5-dev


Re: [gem5-dev] Review Request: inorder/dtb: make sure DTB translate correct address

2011-06-09 Thread Ali Saidi


 On 2011-06-09 11:17:24, Gabe Black wrote:
  This isn't a review, just a thought on the question you're asking. If the 
  access is speculative, is it ok to use a misspeculated pc since the 
  instruction will be thrown out anyway?
 
 Steve Reinhardt wrote:
 Actually after briefly looking at the code, I wonder if we don't have a 
 bigger problem here.  I'm assuming that the ThreadContext struct reflects the 
 committed state of the machine, where we really want the state relative to 
 this in-flight instruction.  Have we just been getting lucky so far?  Seems 
 like in Alpha the TLB only uses the PC to see if it's in PAL mode or not, so 
 maybe the pipeline gets flushed when we switch in and out of PAL mode and 
 thus there's never an inconsistency there even in O3.  (Nate or Ali, can you 
 confirm this?)
 
 Korey, can you elaborate on the kind of errors you were running into 
 without this change?
 
 Technically I think perhaps the TLB should be using the ExecContext 
 interface instead of the ThreadContext, but since ExecContext isn't a real 
 object, that only works if you template the call... I wonder if we really 
 should be using something like the ProxyThreadContext (see 
 cpu/thread_context.hh) wrapped around a DynInst.  Does that make sense?


Steve, you're correct that entrances and exists from PAL are serialize the 
pipeline so it isn't an issue. 

The thread context is pretty much used to just read miscellaneous register. Due 
to them (rightly) not being renamed and updated at commit the tlb is getting 
non-speculative state only. Any instruction that is going to do something that 
needs to be visible to later instructions needs to be marked as serializing. In 
short, I don't think we need to change the current interface and the hwrei 
change is very broken.


- Ali


---
This is an automatically generated e-mail. To reply, visit:
http://reviews.m5sim.org/r/743/#review1311
---


On 2011-06-08 23:34:50, Korey Sewell wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://reviews.m5sim.org/r/743/
 ---
 
 (Updated 2011-06-08 23:34:50)
 
 
 Review request for Default, Ali Saidi, Gabe Black, Steve Reinhardt, and 
 Nathan Binkert.
 
 
 Summary
 ---
 
 inorder/dtb: make sure DTB translate correct address
 The DTB expects the correct PC in the ThreadContext
 but how if the memory accesses are speculative? Shouldn't
 we send along the requestor's PC to the translate functions?
 
 
 Diffs
 -
 
   src/cpu/inorder/resources/cache_unit.cc 77d12d8f7971 
 
 Diff: http://reviews.m5sim.org/r/743/diff
 
 
 Testing
 ---
 
 
 Thanks,
 
 Korey
 


___
gem5-dev mailing list
gem5-dev@m5sim.org
http://m5sim.org/mailman/listinfo/gem5-dev


Re: [gem5-dev] Review Request: inorder/dtb: make sure DTB translate correct address

2011-06-09 Thread Korey Sewell


 On 2011-06-09 11:17:24, Gabe Black wrote:
  This isn't a review, just a thought on the question you're asking. If the 
  access is speculative, is it ok to use a misspeculated pc since the 
  instruction will be thrown out anyway?
 
 Steve Reinhardt wrote:
 Actually after briefly looking at the code, I wonder if we don't have a 
 bigger problem here.  I'm assuming that the ThreadContext struct reflects the 
 committed state of the machine, where we really want the state relative to 
 this in-flight instruction.  Have we just been getting lucky so far?  Seems 
 like in Alpha the TLB only uses the PC to see if it's in PAL mode or not, so 
 maybe the pipeline gets flushed when we switch in and out of PAL mode and 
 thus there's never an inconsistency there even in O3.  (Nate or Ali, can you 
 confirm this?)
 
 Korey, can you elaborate on the kind of errors you were running into 
 without this change?
 
 Technically I think perhaps the TLB should be using the ExecContext 
 interface instead of the ThreadContext, but since ExecContext isn't a real 
 object, that only works if you template the call... I wonder if we really 
 should be using something like the ProxyThreadContext (see 
 cpu/thread_context.hh) wrapped around a DynInst.  Does that make sense?

 
 Ali Saidi wrote:
 Steve, you're correct that entrances and exists from PAL are serialize 
 the pipeline so it isn't an issue. 
 
 The thread context is pretty much used to just read miscellaneous 
 register. Due to them (rightly) not being renamed and updated at commit the 
 tlb is getting non-speculative state only. Any instruction that is going to 
 do something that needs to be visible to later instructions needs to be 
 marked as serializing. In short, I don't think we need to change the current 
 interface and the hwrei change is very broken.

Can you rephrase this: I don't think we need to change the current interface 
and the hwrei change is very broken. Are you saying that the subsequent hwrei 
patch is not what we want or ???

I think the interface is broken and we get away with it more than it's 
correctly coded. We may get away with in alpha, but I would think seeding the 
TLB with the last committed PC rather than the current  PC that needs to be 
translated is wrong and would at some point break the code.

Steve,
I think when I caught this error it was when the InOrder wasnt respecting the 
serializeBefore flag correctly. Thus, you get a mixup in which instruction PC 
needs to be translated.


- Korey


---
This is an automatically generated e-mail. To reply, visit:
http://reviews.m5sim.org/r/743/#review1311
---


On 2011-06-08 23:34:50, Korey Sewell wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://reviews.m5sim.org/r/743/
 ---
 
 (Updated 2011-06-08 23:34:50)
 
 
 Review request for Default, Ali Saidi, Gabe Black, Steve Reinhardt, and 
 Nathan Binkert.
 
 
 Summary
 ---
 
 inorder/dtb: make sure DTB translate correct address
 The DTB expects the correct PC in the ThreadContext
 but how if the memory accesses are speculative? Shouldn't
 we send along the requestor's PC to the translate functions?
 
 
 Diffs
 -
 
   src/cpu/inorder/resources/cache_unit.cc 77d12d8f7971 
 
 Diff: http://reviews.m5sim.org/r/743/diff
 
 
 Testing
 ---
 
 
 Thanks,
 
 Korey
 


___
gem5-dev mailing list
gem5-dev@m5sim.org
http://m5sim.org/mailman/listinfo/gem5-dev