Select legal operand for "m" and "ZC" constraints #38
Conversation
For "m" and "ZC" constraints, 9-bit offsets can be handled. Also, the two least-significant bits should be cleared as required by ll and sc instructions for NanoMips.
| return false; | ||
| } | ||
| } | ||
| [[fallthrough]]; | ||
| case InlineAsm::Constraint_o: |
There was a problem hiding this comment.
Wouldn't this apply to 'o' as a constraint too?
There was a problem hiding this comment.
I am not completely sure if the "m" and "o" constraints should follow the same logic in the case of NanoMips architecture. It seems that GCC doesn't allow any offsets regarding the "m" constraint, while it allows 13-bit offsets for "o". If we want to adhere to this logic, we may need to separate conditions for Constraint_o and Constraint_m.
| // On NanoMips, they can only handle 9-bit offsets. Also, 2 | ||
| // least-significant bits need to be cleared as required by LL and SC | ||
| // instructions. | ||
| if (selectAddrRegImm9(Op, Base, Offset)) { |
There was a problem hiding this comment.
This will verify that the offset is 4-byte aligned? Better move this to Constraint_o. Also on gcc m constraint doesn't allow offsets. While i disagree with that decision it is better to follow gcc here.
There was a problem hiding this comment.
I see LL_NM and SC_NM are wrongly defined to use mem_simm9. Lets not make the same mistake here.
There was a problem hiding this comment.
I agree the current solution isn't enough to ensure a 4-byte alignment. I will make sure to fix this.
Regarding the "o" constraint, it seems that to follow GCC logic, we may need to use something like this:
if (selectAddrFrameIndexOffset(Op, Base, Offset, 13)) {
OutOps.push_back(Base);
OutOps.push_back(Offset);
return false;
}
Also, with larger offsets, GCC separates the constant in the following manner.
For example, if we have a constant 4098, GCC generates the code as follows:
addiu $a0,$a0,4096
ll $zero, 2($a0)
Should we follow this convention, or
addiu $a0,$a0,4098
ll $zero, 0($a0)
would be sufficient?
There was a problem hiding this comment.
For large offset just keep current clang behavior. For "o" I would thought that gcc would choose 9-bit signed offsets as that would be the lowers common denominator of "general" load/store instructions. @farazs-github Would this be an bug on gcc part? Or maybe non-symmetrical 12bit-unsigned 9bit negative range. Not sure.
| // On NanoMips, they can only handle 9-bit offsets. Also, 2 | ||
| // least-significant bits need to be cleared as required by LL and SC | ||
| // instructions. | ||
| if (selectAddrRegImm9(Op, Base, Offset)) { |
There was a problem hiding this comment.
selectAddrFrameIndexOffset(Op, Base, Offset, 7, 2)?
milica-lazarevic
force-pushed
the
illegal-operand-selected-for-m-zc
branch
from
e3a34bc to
f9a7024
Compare
There was a problem hiding this comment.
LGTM. We had an request to include tests here as well. @farazs-github Does it make sense to follow gcc here for "o" constraint?
| // On NanoMips, they can only handle 9-bit offsets. Also, 2 | ||
| // least-significant bits need to be cleared as required by LL and SC | ||
| // instructions. | ||
| if (selectAddrRegImm9(Op, Base, Offset)) { |
There was a problem hiding this comment.
I see LL_NM and SC_NM are wrongly defined to use mem_simm9. Lets not make the same mistake here.
| // On NanoMips, they can only handle 9-bit offsets. Also, 2 | ||
| // least-significant bits need to be cleared as required by LL and SC | ||
| // instructions. | ||
| if (selectAddrRegImm9(Op, Base, Offset)) { |
There was a problem hiding this comment.
For large offset just keep current clang behavior. For "o" I would thought that gcc would choose 9-bit signed offsets as that would be the lowers common denominator of "general" load/store instructions. @farazs-github Would this be an bug on gcc part? Or maybe non-symmetrical 12bit-unsigned 9bit negative range. Not sure.
In NanoMips, for the following constraints, these conditions need to be fulfilled: 'm' - don't allow any offsets, 'o' - allow 13bit offsets, 'ZC' - allow 9bit 4byte aligned offsets.
milica-lazarevic
force-pushed
the
illegal-operand-selected-for-m-zc
branch
from
f9a7024 to
de65e89
Compare
Fixes a bug introduced by merge. Change-Id: I48499b4afd66b090b1e6b7d6b59993d65001e534
For "m" and "ZC" constraints, 9-bit offsets can be handled. Also, the two least-significant bits should be cleared as required by ll and sc instructions for NanoMips.