Alpha specific alignment of PC

Information
Submitter:	Anders Handler
Repository:	gem5
Branch:
Bugs:
Depends On:
Reviewers
Groups:	Default
People:

Description

Alignment of PC is ALPHA specific, thus checks for ISA is needed.

Testing Done

Hi Andres,

Thanks for taking the time to do this and submit it. I'm not a huge fan of ifdefs in the tree, so I think it would be a lot better if we created a const Addr in isa_traits.hh for every architecture and set it to either ~ULL(3) or ~ULL(0). That leaves the option open in the future for making the constant a variable if we ever get around to making a single binary that can simulate all the architectures.

Thanks again,
Ali

Ali Saidi Dec. 21, 2011, 6:38 a.m. (Dec. 21, 2011, 6:38 a.m.)
```
Oops, I mean Anders.
```

Change Summary:

Sure. All isa_traits.hh updated as well as the pc_event.cc.

Diff:

Revision 2 (+21 -2)

Show changes

	src/arch/alpha/isa_traits.hh
	src/arch/arm/isa_traits.hh
	src/arch/mips/isa_traits.hh
	src/arch/power/isa_traits.hh
	src/arch/sparc/isa_traits.hh
	src/arch/x86/isa_traits.hh
	src/cpu/pc_event.cc

Ali, can you take care of it?

Gabe Black Dec. 22, 2011, 5:51 a.m. (Dec. 22, 2011, 5:51 a.m.)

I really don't want things implemented this way. The fact that Alpha is an aberration and doesn't work like it's supposed to should be codified by building support for it into all the ISAs. It should be left as an Alpha specific hack (there is plenty of precedent) and cleaned up when possible. The we won't have to rip support back out of all the other ISAs, or worry about where else this misfeature has crept into.

Gabe Black Dec. 22, 2011, 5:54 a.m. (Dec. 22, 2011, 5:54 a.m.)

should be codified => shouldn't be codified

Sorry to yank you back and forth on this Anders.

Nathan Binkert Dec. 22, 2011, 6:02 a.m. (Dec. 22, 2011, 6:02 a.m.)

What do you mean Alpha "doesn't work like it's supposed to".  It works how it works.  The only time you're going to "rip support back out" is if we *remove* alpha.  That's unlikely to happen any time soon if ever.  What's more likely is that we eventually have support for multiple ISAs in one binary and IMHO this is way better than a #ifdef.  My preference would be to *never* have an ISA specific #ifdef in the tree.

I feel very strongly about this.  If you look at how other software systems (linux, *bsd, etc.) work, they follow this idiom.

Gabe Black Dec. 22, 2011, 6:09 a.m. (Dec. 22, 2011, 6:09 a.m.)

I think this may be the third time explaining this, but no, I'm not talking about ripping out Alpha. The fact that the PAL bit shows up when you can instAddr is wrong. It's that way because O3 wouldn't work otherwise, and we had a lengthy conversation about that a while ago. Recently when Anders wrote the list about this specific patch I also explained how this was wrong. Alpha doesn't work the way it's supposed to, and when it's fixed *this* support will need to be ripped back out. I don't want a hack to work around a deficiency of O3 to turn into a full fledged feature. Also, if you look at the snippet of code I called out in my most recent email, you can see how to do this conditionally without an ifdef.

Nathan Binkert Dec. 22, 2011, 6:15 a.m. (Dec. 22, 2011, 6:15 a.m.)

I guess my preference would be that we select this patch if the fix that you're talking about is a few months out (which has a habit of becoming forever).  If you truly do have plans to fix this (more than a belief that it would be nice to have).  Then I'd be happy to have the one you prefer.  I'm also happy to have a comment saying that the constant should be ripped out of the ISAs if it is ever removed from O3.  I also wouldn't mind putting O3 in the name to make it more obvious.

Anders Handler Jan. 1, 2012, 5:38 p.m. (Jan. 1, 2012, 5:38 p.m.)
```
What was the conclusion of this discussion?
```

Ali Saidi Jan. 5, 2012, 9:24 a.m. (Jan. 5, 2012, 9:24 a.m.)

To just turn  Addr pc = tc->instAddr() & ~0x3; -> into Addr pc = tc->instAddr() and same for the similar line below it.

It breaks one specific case in Alpha, but we're fine with that.

Sorry for all the confusion and back and forth.

You have a pending review.

Review Board 2.0.15

This change has been marked as submitted.

Screenshots

Files

Change Summary:

Diff: