Created attachment 398068 [details] [diff] [review] patch We see this pattern a lot in the Assembler: Reservation* resv = getresv(ins); NanoAssert(resv); ... use resv->reg, etc ... getresv() is inlined, so it becomes this: Reservation* resv = ins->resv(); resv = (r->used ? r : 0); NanoAssert(resv); ... use resv->reg, etc ... In optimised builds the assert is removed: Reservation* resv = ins->resv(); resv = (r->used ? r : 0); ... use resv->reg, etc ... The r->used test should always succeed (if it doesn't, there's a bug which a debug build should catch). This patch removes all occurrences of this idiom in Assembler.cpp and Nativei386.cpp (but not the other back-ends), replacing them with this: Reservation* resv = ins->resv(ins); NanoAssert(resv->used); ... use resv->reg, etc ... thus avoiding the unnecessary test. (A couple of the modified places didn't have the assertion, but the code relied on resv->used being true.) Furthermore, I introduced resvUsed() which does the assert itself, so now you actually see this: Reservation* resv = ins->resvUsed(ins); ... use resv->reg, etc ... I also changed other "resv = getresv(ins)... if (resv)" occurrences to "resv = ins->resv()... if (resv->used)" for consistency and readability. I've run SS several times and am seeing a 0--3ms speedup. I also checked the generated asm and it's several instructions fewer for each occurrence and so is a clear win in terms of code size.
Attachment #398068 - Flags: review?(gal)
Looks great, but I am not a big fan of the resvUsed() name. I think I prefer the explicit assert. It helps reading the code if its explicit there, at least for me. If you find a better name for resvUsed, I would be ok with that too. Any ideas?
Earlier I was using resv() for the checking version, and resvMaybeUnused() for the unchecked version -- is that any better? Or if you want more verbosity: getUsedResv()?
getUsedResv sounds pretty cheesy too. I don't have any great ideas either though. resvOrNull? I still like resv() and explicit asserts best.
resvOrNull() matches the old behaviour -- resvOrAssert() would be more appropriate. The reason I want to remove the assertion is that if you look through Assembler.cpp and the back-ends there is an amazing amount of code dealing with reservations when really a reservation is a middle-man -- what we're really interested in is the reg/used/arIndex fields. I want to reduce some of this noise, it obscures the interesting work done by the code. Maybe functions like ins->getReg() and ins->setReg() would be better. But for the moment I'll probably just put the asserts back in.
Yeah, I agree. getresv() is a leftover of when there was a mapping table in between. Maybe we should get rid of the Reservation struct.
Comment on attachment 398068 [details] [diff] [review] patch Looks good. Handle resv*() as you see fit. All options are about equally ugly.
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
status1.9.2: --- → beta1-fixed
You need to log in before you can comment on or make changes to this bug.