nathan binkert wrote:
>> diff --git a/src/arch/x86/isa/microops/ldstop.isa
>> b/src/arch/x86/isa/microops/ldstop.isa
>> --- a/src/arch/x86/isa/microops/ldstop.isa
>> +++ b/src/arch/x86/isa/microops/ldstop.isa
>> @@ -454,7 +454,7 @@
>> Mem = Data;
>> Base = merge(Base, EA - SegBase, addressSize);
>> ''');
>> -
>> + defineMicroStoreOp('Cda', 'Mem = 0;', "Request::NO_ACCESS")
>>
>> iop = InstObjParams("lea", "Lea", 'X86ISA::LdStOp',
>> {"code": "Data = merge(Data, EA, dataSize);",
>> @@ -493,17 +493,6 @@
>>
>> microopClasses["tia"] = TiaOp
>>
>> - iop = InstObjParams("cda", "Cda", 'X86ISA::LdStOp',
>> - {"code": '''
>> - Addr paddr;
>> - fault = xc->translateDataWriteAddr(EA, paddr,
>> - dataSize, (1 << segment));
>> - ''',
>> - "ea_code": calculateEA})
>> - header_output += MicroLeaDeclare.subst(iop)
>> - decoder_output += MicroLdStOpConstructor.subst(iop)
>> - exec_output += MicroLeaExecute.subst(iop)
>> -
>> class CdaOp(LdStOp):
>> def __init__(self, segment, addr, disp = 0,
>> dataSize="env.dataSize", addressSize="env.addressSize"):
>>
>
> Is the above section really related to this diff?
>
Yes. I'm reimplementing the cda microop to not use the
translateDataWriteAddr function. It's not inseparably entwined but it
fits wellish. It would be a pain to pull that part out.
>
>> @@ -889,7 +860,7 @@
>> req->setVirt(asid, addr, sizeof(T), flags, this->PC);
>> req->setThreadContext(thread->contextId(), threadNumber);
>>
>> - fault = cpu->translateDataReadReq(req, thread);
>> + fault = cpu->dtb->translate(req, thread->getTC(), false);
>>
>> if (req->isUncacheable())
>> isUncacheable = true;
>>
>
> Is the indentation above correct? It looks different, but I'm looking
> at it variable width. Are you inserting tabs or something? Maybe
> it's just the difference in width between + and -
>
It at least looks consistent between the + and - to me. There seems to
be one more character than the other unmodified lines but who knows what
that's from. I at least didn't add any new brokeness. The patch itself
is fine.
>
>> @@ -158,8 +158,9 @@
>>
>> memReq->setVirt(0, addr, sizeof(T), flags, thread->readPC());
>>
>> +
>> // translate to physical address
>> - translateDataReadReq(memReq);
>> + dtb->translate(memReq, tc, false);
>>
>> PacketPtr pkt = new Packet(memReq, Packet::ReadReq, Packet::Broadcast);
>>
>>
> Evil random whitespace above.
>
>
Fixed.
>> @@ -497,15 +493,19 @@
>> if (fault != NoFault)
>> return fault;
>> dcache_pkt = pkt1;
>> - if (handleWritePacket()) {
>> - SplitFragmentSenderState * send_state =
>> - dynamic_cast<SplitFragmentSenderState *>(pkt1->senderState);
>> - send_state->clearFromParent();
>> - dcache_pkt = pkt2;
>> - if (handleReadPacket(pkt2)) {
>> - send_state =
>> - dynamic_cast<SplitFragmentSenderState
>> *>(pkt1->senderState);
>> + if (!req->getFlags().isSet(Request::NO_ACCESS)) {
>> + if (handleWritePacket()) {
>> + SplitFragmentSenderState * send_state =
>> + dynamic_cast<SplitFragmentSenderState *>(
>> + pkt1->senderState);
>> send_state->clearFromParent();
>> + dcache_pkt = pkt2;
>> + if (handleReadPacket(pkt2)) {
>> + send_state =
>> + dynamic_cast<SplitFragmentSenderState *>(
>> + pkt1->senderState);
>> + send_state->clearFromParent();
>> + }
>> }
>> }
>> } else {
>> @@ -515,21 +515,23 @@
>> if (fault != NoFault)
>> return fault;
>>
>> - if (req->isLocked()) {
>> - do_access = TheISA::handleLockedWrite(thread, req);
>> - } else if (req->isCondSwap()) {
>> - assert(res);
>> - req->setExtraData(*res);
>> + if (!req->getFlags().isSet(Request::NO_ACCESS)) {
>> + if (req->isLocked()) {
>> + do_access = TheISA::handleLockedWrite(thread, req);
>> + } else if (req->isCondSwap()) {
>> + assert(res);
>> + req->setExtraData(*res);
>> + }
>> +
>> + dcache_pkt->allocate();
>> + if (req->isMmapedIpr())
>> + dcache_pkt->set(htog(data));
>> + else
>> + dcache_pkt->set(data);
>> +
>> + if (do_access)
>> + handleWritePacket();
>> }
>> -
>> - dcache_pkt->allocate();
>> - if (req->isMmapedIpr())
>> - dcache_pkt->set(htog(data));
>> - else
>> - dcache_pkt->set(data);
>> -
>> - if (do_access)
>> - handleWritePacket();
>> }
>>
>> if (traceData) {
>>
> You're starting to get a lot of indentation here. Can you exit early,
> or use a goto to make it look better?
>
I'm not crazy about goto here because it could interfere with compiler
optimization on a fairly hot code path. This gets a major overhaul in
patch 5 anyway, so I'd prefer to leave it a little overly indented for
the short span between the two. Coincidentally the change in 5 does move
the heavily indented parts, the actual access, into a different function
which is called when the translation completes.
Gabe
_______________________________________________
m5-dev mailing list
[email protected]
http://m5sim.org/mailman/listinfo/m5-dev
