Respect the sign of the right operand of AST_SHIFT and AST_SHIFTX

[daglem]

The $shift and $shiftx cells perform a left logical shift if the second operand is negative. This change passes the sign of the second operand of AST_SHIFT and AST_SHIFTX into $shift and $shiftx cells, respectively.

[povik]

As I understand this change it's not so much that the second operand to AST_SHIFT/AST_SHIFTX couldn't be negative otherwise, it's that it causes the second operand to be determined to be signed in more cases. In any case those AST nodes are for internal use only and we are free to tune their definition.

[daglem]

As I understand this change it's not so much that the second operand to AST_SHIFT/AST_SHIFTX couldn't be negative otherwise, it's that it causes the second operand to be determined to be signed in more cases. In any case those AST nodes are for internal use only and we are free to tune their definition.

Thanks for merging!

Before this change, the second operand was converted to unsigned before being passed to the $shift / $shiftx cell. The change facilitates use of AST_SHIFTX in #3877 to replace the current complicated "fake_ast" code in genrtlil.cc, and I suspect that without the change, it would not be possible to use AST_SHIFT without using temporaries as is currently done in the bit slice assignment in simplify.cc. I'll make a PR to fix #4064 shortly (on top of #3875), using AST_SHIFT without the use of temporaries.

[povik]

Before this change, the second operand was converted to unsigned before being passed to the $shift / $shiftx cell.

Well, that depends on the operand, doesn't it? binop2rtlil which emits the cells themselves will use the operand's AST node's ->is_signed, and the is_signed attribute is determined as a function of both the sign_hint (which we now flipped on), and what kind of expression that second operand was.

[daglem]

Before this change, the second operand was converted to unsigned before being passed to the $shift / $shiftx cell.

Well, that depends on the operand, doesn't it? binop2rtlil which emits the cells themselves will use the operand's AST node's ->is_signed, and the is_signed attribute is determined as a function of both the sign_hint (which we now flipped on), and what kind of expression that second operand was.

I don't think that's correct. As far as I understand, the previous call to children[1]->genRTLIL() would always make children[1] unsigned (is_signed = false), since sign_hint in the call to genRTLIL() must be true for the node to end up signed. Then, the sign of children[1] was used for the second operand to the $shift/ $shiftx, which would then also be unsigned. I verified this by looking at the generated RTLIL.

[povik]

since sign_hint in the call to genRTLIL() must be true for the node to end up signed.

Reading more into the code, I think you are right. At least not even $signed() will make the operand signed if the sign_hint is false. Maybe a wire could force signedness.

[daglem]

since sign_hint in the call to genRTLIL() must be true for the node to end up signed.

Reading more into the code, I think you are right. At least not even $signed() will make the operand signed if the sign_hint is false. Maybe a wire could force signedness.

Yes, that's what I suspect is happening for the current use of AST_SHIFT in the bit slice assignment code in simplify.cc.