Set isInternalRegDelayFree for several of the x86 hwintrinsics#16649
Conversation
| emit->emitDataGenEnd(); | ||
|
|
||
| // Ensure we aren't overwriting offsReg, baseReg, or nonConstImmReg | ||
| assert(offsReg != baseReg); |
There was a problem hiding this comment.
Is there any guarantee that two temporary registers won't be the same? Do I need to set isInternalRegDelayFree to guarantee that?
There was a problem hiding this comment.
Temporary registers have identical live ranges so they cannot be the same. If their live ranges would be disjoint then you'd probably need only one temporary register to begin with.
| regNumber offsReg = node->GetSingleTempReg(); | ||
|
|
||
| // Ensure we aren't overwriting op1Reg | ||
| assert(baseReg != op1Reg); |
There was a problem hiding this comment.
@CarolEidt, you mentioned
It's only needed if the tempReg needs to be different from the target. The internal registers never conflict with incoming sources.
Is the second part (never conflict with incoming sources) also true in the case where a source register is the same as the target register?
There was a problem hiding this comment.
in the case where a source register is the same as the target register?
Isn't this the same as being RMW?
There was a problem hiding this comment.
Basically.
But my question is that:
- since
tmpRegcan equaltargetReg(whenisInternalRegDelayFree=false) - and since
opRegcan equaltargetReg(whenisDelayFree=false) - is it possible that
tmpRegcan equalopReg(whenopReg == targetReg)?- or is it a hard limitation that
tmpReg != opReg(based on @CarolEidt's statement)?
- or is it a hard limitation that
There was a problem hiding this comment.
No, tmpReg can't equal opReg. Temporary registers are treated as if they are defined before operands are used so they have to be different.
I think I've seen these details described somewhere in the documentation but I can't find that right now.
There was a problem hiding this comment.
Ah, it wasn't in the documentation, it's in the comment at the start of lsra.cpp:
Lines 24 to 39 in f1fee6d
There was a problem hiding this comment.
@mikedn - thanks for adding all clarification (I'm currently out of the country so my responses are slow and off-timezone)
| assert(baseReg != op1Reg); | ||
| assert(baseReg != op2Reg); | ||
| assert(offsReg != op1Reg); | ||
| assert(offsReg != op2Reg); |
There was a problem hiding this comment.
IMO these assertions are a bit of overkill, but I guess they can't hurt.
There was a problem hiding this comment.
I had initially added them because I was unsure if tmpReg can ever be opReg (#16649 (comment)).
I am going to remove these "unneeded" asserts as part of the rebase.
|
Removed the unnecessary asserts (for the case where |
|
test Windows_NT x64 Checked jitincompletehwintrinsic test Windows_NT x86 Checked jitincompletehwintrinsic test Ubuntu x64 Checked jitincompletehwintrinsic test OSX10.12 x64 Checked jitincompletehwintrinsic |
FYI. @CarolEidt, @mikedn, @fiigii. As per the discussion here: #16558 (comment)