Bug 2809 – Wrong constant folding for unsigned shift

Status
RESOLVED
Resolution
INVALID
Severity
normal
Priority
P2
Component
dmd
Product
D
Version
D1 (retired)
Platform
x86
OS
Windows
Creation time
2009-04-06T09:05:17Z
Last change time
2020-08-09T01:41:12Z
Keywords
patch, wrong-code
Assigned to
No Owner
Creator
anonymous4
Depends on
3147, 5225

Comments

Comment #0 by dfj1esp02 — 2009-04-06T09:05:17Z
--- const short s=-1; static assert(s>>>1==0x7fff); //fail --- Influenced by error messages, where compiler transforms a>>>b to cast(int)a>>>b. Here compiler complains about conversion to return type: --- short a(short b) { return b>>>1; } --- When you add it, the code is accepted, but the bug is already triggered. --- short a(short b) { return cast(short)(b>>>1); } ---
Comment #1 by clugdbug — 2010-01-16T23:37:20Z
Also applies to D1. Seems to be a constant folding issue.
Comment #2 by clugdbug — 2010-01-17T11:48:08Z
Now I'm really confused. In Walter's test suite, this case is explicitly tested! static assert((cast(short)-1 >>> 1) == int.max); There's a wrong statement in the bug description. "Here compiler complains about conversion to return type: --- short a(short b) { return b>>>1; } " the response to this should be: short a(short b) { return b>>>cast(short)1; } So I'm rather confused about whether this is a bug, or intended (but unintuitive) behaviour. If it's a bug, it can be fixed by modifying UshrExp::semantic(Scope *sc) in expression.c (around line 10103): e1 = e1->checkIntegral(); e2 = e2->checkIntegral(); - e1 = e1->integralPromotions(sc); + e = e1->integralPromotions(sc); e2 = e2->castTo(sc, Type::tshiftcnt); - type = e1->type; + type = e->type; } return this; and in constfold.c Ushr(), around line 600, removing two asserts. case Tint8: case Tuns8: - assert(0); // no way to trigger this value = (value & 0xFF) >> count; break; case Tint16: case Tuns16: - assert(0); // no way to trigger this value = (value & 0xFFFF) >> count; break;
Comment #3 by s.d.hammett — 2010-11-15T14:29:19Z
Mr Bs test case is wrong: static assert((cast(short)-1 >>> 1) == int.max); should be: static assert((cast(short)-1 >>> 1) == short.max); unsigned right shift is perfectly well defined, though giving it it's own operator seems like overkill. I think it would be better as a function in std.intrinsic. You aren't going to use unsigned shift unless you know what you doing and care about performance.
Comment #4 by clugdbug — 2010-11-15T15:06:34Z
(In reply to comment #3) > Mr Bs test case is wrong: > > static assert((cast(short)-1 >>> 1) == int.max); > > should be: > > static assert((cast(short)-1 >>> 1) == short.max); Not so. You might be thinking of this, which _is_ true: static assert((cast(short)-1 >>> cast(short)1) == short.max); The problem is that >>> interacts badly with implicit type conversions. With every other operator, typeof(short OP int) == int. Possible solutions are: (a) special case for >>> (b) disallow >>> for types smaller than int (c) drop it from the language Personally I think (c) is the only option that makes sense. > unsigned right shift is perfectly well defined, > though giving it it's own operator seems like overkill. > > I think it would be better as a function in std.intrinsic. You don't need it at all. Just cast to unsigned, then >>. >>> is a ridiculous operator. > You aren't going to use unsigned shift unless you know what you doing and care > about performance.
Comment #5 by dfj1esp02 — 2010-11-16T11:53:02Z
>short a(short b) { return b>>>cast(short)1; } Shouldn't number literals work as smallest possible type and promoted as needed? Like here: --- byte a=1; ---
Comment #6 by dfj1esp02 — 2010-11-16T11:59:34Z
Number literals are polysemous, right? So binary ops should work like this: opBinary(l,r) { if(is(typeof(r)==polysemous)) { opBinary(l,implicit_cast(typeof(l))r); } } or something like that.
Comment #7 by dfj1esp02 — 2010-11-16T12:34:12Z
> You don't need it at all. Just cast to unsigned, then >>. > >>> is a ridiculous operator. See bug 5225.
Comment #8 by bugzilla — 2020-08-09T01:41:12Z
Since operands always undergo integral promotions, this behavior is expected and correct. The idea is that the expressions will compute the same value as the equivalent C expression in order to enable simple conversion of C code to D code. See: https://dlang.org/spec/type.html#integer-promotions https://dlang.org/spec/type.html#usual-arithmetic-conversions