Minres / CoreDSL

Xtext project to parse CoreDSL files
Apache License 2.0
16 stars 3 forks source link

Clarify sign-extension #58

Closed wysiwyng closed 2 years ago

wysiwyng commented 2 years ago

The current specification (https://github.com/Minres/CoreDSL/wiki/Types#casting) does not show examples of how a sign-extending typecast should look like. From how I read the specification, this example (from https://github.com/Minres/RISCV_ISA_CoreDSL/blob/726fb8117b47714ee891f89032fb8a9ce167b1e2/RV32I.core_desc#L9-L13) would NOT sign extend imm to XLEN bits but instead zero-extend it to XLEN first, then do the signedness conversion:

LUI {
    encoding: imm[31:12] :: rd[4:0] :: 7'b0110111;
    assembly: "{name(rd)}, {imm:#05x}";
    behavior: if ((rd % RFS) != 0) X[rd % RFS] = (signed<XLEN>)imm;
}
eyck commented 2 years ago

In case of XLEN==64 you are right, in case of XLEN==32 i doesn't matter ;-) I would see this as an issue in RISCV_ISA_CoreDSL

jopperm commented 2 years ago

[...] would NOT sign extend imm to XLEN bits but instead zero-extend it to XLEN first, then do the signedness conversion

Correct, we do this to preserve the value while extending. Example: (signed<6>) 4'd15 shall become 6'd15, and not 6-bit -8.

So in order to reinterpret the encoding field as signed, extend it, and then assign it to an unsigned destination of the same width, you have to be (very) explicit about the intended action:

X[rd % RFS] = (unsigned<XLEN>) ((signed) imm));

But yeah, this definitively should be discussed in the docs. I'll clarify it there.

jopperm commented 2 years ago

Changes to docs:

https://github.com/Minres/CoreDSL/wiki/Types/_compare/168aebe6cdb3e622c9ab6c4693d290e8ef36492a...7fd33432ea3d8b126284f6a854f6134418540d10

I initially thought that it would have to be

X[rd % RFS] = (unsigned) ((signed<XLEN>) ((signed) imm)));

but that's actually a longer way of saying (unsigned<XLEN>). I'm using the shorter form in the docs, and updated my comment above.

wysiwyng commented 2 years ago

So in order to reinterpret the encoding field as signed, extend it, and then assign it to an unsigned destination of the same width, you have to be (very) explicit about the intended action:

Technically, wouldn't it also be correct to do just:

X[rd % RFS] = (signed)imm;

As implicit extension is perfectly valid per the spec?

jopperm commented 2 years ago

As implicit extension is perfectly valid per the spec?

True, but here it's assigning a signed value to an unsigned destination, hence requiring an explicit cast to acknowledge the loss of sign info.