cache: don't allow dirty data in the i-cache

Information
Submitter:	Tony Gutierrez
Repository:	gem5
Branch:	default
Bugs:
Depends On:
Reviewers
Groups:	Default
People:

Description

Changeset 9086:8a09d8787f5a
---------------------------
cache: don't allow dirty data in the i-cache

removes the optimization that forwards an exclusive copy to a requester on a
read, only for the i-cache. this optimization isn't necessary because we
typically won't be writing to the i-cache.

Testing Done

Added i-cache invalidation instruction for ARM. These instructions invalidate i-cache entries without writing them back, because it is not expected for there to be dirty data in the caches. While invalidating I would get assert failures and/or the program would hang due to dirty data in the i-cache being invalidated. Doing this fixed those problems. Ran BBench to completion with this fix and inv instructions in place. This is a necessary step towards adding the inv instructions for ARM.

Summary:

-	caches: prevent dirty data in the i-cache
+	cache: don't allow dirty data in the i-cache

Description:

~		Changeset 9086:66e48e50b36c
	~	Changeset 9086:1c54688932a1

		cache: don't allow dirty data in the i-cache

		removes the optimization that forwards an exclusive copy to a requester on a
		read, only for the i-cache. this optimization isn't necessary because we
		typically won't be writing to the i-cache.

Diff:

Revision 2 (+1 -1)

Show changes

src/mem/cache/cache_impl.hh

looks reasonable to me.

Testing Done:

Added i-cache invalidation instruction for ARM. These instructions invalidate i-cache entries without writing them back, because it is not expected for there to be dirty data in the caches. While invalidating I would get assert failures and/or the program would hang due to dirty data in the i-cache being invalidated. Doing this fixed those problems. Ran BBench to completion with this fix and inv instructions in place. This is a necessary step towards adding the inv instructions for ARM.

It looks reasonable. I am not entirely sure about the distinction of the packets though. What happens for shared I and D? Would it not be better to make it a property of the cache rather than the packet, i.e. a parameter on the class?

Ali Saidi July 2, 2012, 6:29 a.m. (July 2, 2012, 6:29 a.m.)

I think adding a flag on the cache model is more complex and doesn't buy much in practice. Dirty data can still live in a unified cache (because it would get inserted by a write instead of an instruction fetch). The change would need to be a property of the requesting cache, but it would need to effect the sending cache in which case more fields would need to be added to the packet. 

In summary, I still think that the way it's implemented is probably preferable.

Tony Gutierrez July 2, 2012, 6:32 a.m. (July 2, 2012, 6:32 a.m.)

If it's a shared i/d cache, I would assume that a packet that is fetching an instruction would still be distinguished as such. Also, I think a shared i/d cache would cause several other issues beyond this. I.e., we couldn't have the cache invalidation instructions, as they actually function in ARM/x86, modeled in gem5 at all. E.g., we couldn't implement mcr icialluis as it actually works, or if we did, we could never have dirty data in the L1.

Andreas Hansson July 2, 2012, 7:04 a.m. (July 2, 2012, 7:04 a.m.)

All makes sense. Do we have to add some form of check to ensure that the user-specified configuration adheres to specific rules?

Tony Gutierrez July 2, 2012, 7:10 a.m. (July 2, 2012, 7:10 a.m.)

I don't think so. I think any configuration, other than unifying I/D caches, would work just fine with this. If somebody did want a unified I/D cache they would have to make more modifications to gem5 other than configs.

Ali Saidi July 2, 2012, 7:14 a.m. (July 2, 2012, 7:14 a.m.)

I'm pretty sure unified I/D will work too because while ownership wont get transfered on an instruction read, it would get transfered on an data write. Instruction cache flushing won't work, but at this point it doesn't work now anyway. :) Are you happy with this change Andreas?

Tony Gutierrez July 2, 2012, 7:20 a.m. (July 2, 2012, 7:20 a.m.)

Right, any configuration should work with this. I was referring to a situation where invalidation works and is implemented.

Andreas Hansson July 2, 2012, 6:32 p.m. (July 2, 2012, 6:32 p.m.)

I'm happy, assuming the comments are updated to reflect the change :)

Tony Gutierrez July 2, 2012, 8:35 p.m. (July 2, 2012, 8:35 p.m.)
```
Done.
```

Ship It!

Description:

~		Changeset 9086:1c54688932a1
	~	Changeset 9086:634a0b22e658

		cache: don't allow dirty data in the i-cache

		removes the optimization that forwards an exclusive copy to a requester on a
		read, only for the i-cache. this optimization isn't necessary because we
		typically won't be writing to the i-cache.

Diff:

Revision 3 (+3 -1)

Show changes

src/mem/cache/cache_impl.hh

Description:

~		Changeset 9086:634a0b22e658
	~	Changeset 9086:8a09d8787f5a

		cache: don't allow dirty data in the i-cache

		removes the optimization that forwards an exclusive copy to a requester on a
		read, only for the i-cache. this optimization isn't necessary because we
		typically won't be writing to the i-cache.

Diff:

Revision 4 (+4 -2)

Show changes

src/mem/cache/cache_impl.hh

Ship It!

You have a pending review.

Review Board 2.0.15

This change has been marked as submitted.

Screenshots

Files

Summary:

Description:

Changeset 9086:66e48e50b36c

Changeset 9086:1c54688932a1

Diff:

Testing Done:

Description:

Changeset 9086:1c54688932a1

Changeset 9086:634a0b22e658

Diff:

Description:

Changeset 9086:634a0b22e658

Changeset 9086:8a09d8787f5a

Diff:

Status: Closed (submitted)