Apparently, the signed char b is sign-extended to a 16-bit int. But ds390 uses 24-bit pointers and in the addition, the upper byte is 0. So only the lower and middle byte are correct.
Philipp
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
--- old+++ new@@ -1,3 +1,3 @@-Bug #2601 affected all backends. It is fixed now, but the regression test still fails for ds390.+Bug [#2601] affected all backends. It is fixed now, but the regression test still fails for ds390.
Philipp
Actually, this is a new failure for ds390 (just like the failure of the bug3537338 test) introduced by the fix for bug 2601:
Addition of signed chars to pointers used to work for ds390. But the fix for #2601 introduces a cast to int. And addition of ints to pointers was already broken for ds390.
Philipp
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Would it not be better to make the backends handle addition of a char or int to a pointer? I expect that to be much more efficient than introducing a cast.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
But then genPlus() would have to be able to sign-extend operands before addition. Essentially, genPlus() for each backend would have to be able to do a signed cast on the right operand before addition. I don't see an efficiency advantage vs. having the cast in the iCode.
Philipp
Edit: There would be a minor efficiency advantage for ds390, which has 24-bit pinters, but the explicit cast is a cast to a 32-bit long. It still doesn't seem worth to put the extra burden on all backends. Introducing a 24-bit integer type would help the ds390 here (as well as having other advatages, e.g. in implementing more efficient floats or in having a compliant ptrdiff_t).
Last edit: Philipp Klaus Krause 2017-04-28
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
I expect (haven't checked) the cast to give higher register pressure than letting genPlus() do the sign extension. The downside could be that we can't do CSE on it.
Laziness to implement something in all backends is hardly a good reason if there is efficiency to be gained. But for that we must first find out if it really is more efficient.
I wonder how this fix affected Z80 code with its IX+<nn> access instructions. Isn't <nn> a signed char here?
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Apparently, the signed char b is sign-extended to a 16-bit int. But ds390 uses 24-bit pointers and in the addition, the upper byte is 0. So only the lower and middle byte are correct.
Philipp
Diff:
Related
Bugs:
#2601Actually, this is a new failure for ds390 (just like the failure of the bug3537338 test) introduced by the fix for bug 2601:
Addition of signed chars to pointers used to work for ds390. But the fix for #2601 introduces a cast to int. And addition of ints to pointers was already broken for ds390.
Philipp
Would it not be better to make the backends handle addition of a char or int to a pointer? I expect that to be much more efficient than introducing a cast.
But then genPlus() would have to be able to sign-extend operands before addition. Essentially, genPlus() for each backend would have to be able to do a signed cast on the right operand before addition. I don't see an efficiency advantage vs. having the cast in the iCode.
Philipp
Edit: There would be a minor efficiency advantage for ds390, which has 24-bit pinters, but the explicit cast is a cast to a 32-bit long. It still doesn't seem worth to put the extra burden on all backends. Introducing a 24-bit integer type would help the ds390 here (as well as having other advatages, e.g. in implementing more efficient floats or in having a compliant ptrdiff_t).
Last edit: Philipp Klaus Krause 2017-04-28
I expect (haven't checked) the cast to give higher register pressure than letting genPlus() do the sign extension. The downside could be that we can't do CSE on it.
Laziness to implement something in all backends is hardly a good reason if there is efficiency to be gained. But for that we must first find out if it really is more efficient.
I wonder how this fix affected Z80 code with its IX+<nn> access instructions. Isn't <nn> a signed char here?
Fixed in revision #9887.
Philipp
Is it really too hard to type r9887 between brackets [r9887] instead?