Fix compare immediate and branch helper for 2^n #113523
Conversation
The helper function should only generate `tbz`, when the immediate being compared is a power of 2 and the test is for inequality. Also adds documentation for the helper function.
| @@ -5159,12 +5176,11 @@ void CodeGen::genCompareImmAndJump( | |||
| instruction ins = (cond == GenCondition::EQ) ? INS_cbz : INS_cbnz; | |||
| GetEmitter()->emitIns_J_R(ins, size, target, reg); | |||
| } | |||
| else if (isPow2(compareImm)) | |||
| else if (isPow2((size_t)compareImm) && (cond == GenCondition::NE)) | |||
There was a problem hiding this comment.
What happens when isPow2(compareImm) && (cond == GenCondition::EQ)? In that case, we should be using tbnz, right?
I think the previous code was fine, except the conditions were reversed...If isPow2(), we should have:
instruction ins = (cond == GenCondition::EQ) ? INS_tbnz : INS_tbz;
There was a problem hiding this comment.
I think this transformation is incorrect, it's never correct to change x != 2^i into (x & 2^i) == 0, or vice versa. I don't think we can use tbz or tbnz at all here. For example consider i = 0, x = 3. There we have x != 2^i but we do not have (x & 2^i) == 0.
There was a problem hiding this comment.
We do have a check of compareImm == 0 before we check for isPow2(compareImm), if thats what you meant.
There was a problem hiding this comment.
That transformation looks ok to me. This one doesn't.
The helper function should only generate
tbz, when the immediate being compared is a power of 2 and the test is for inequality.Also adds documentation for the helper function.
This function has only been used so far for internal checks generated in the emitter, so there are no diffs. I found this issue by using the function in some other work.