search

eclipse / omr

Eclipse OMR™ Cross platform components for building reliable, high performance language runtimes
http://www.eclipse.org/omr
Other
940 stars 394 forks source link

Remove instruction that masks the shift amount #2320

Open dchopra001 opened 6 years ago

dchopra001 commented 6 years ago

There are a couple of places in OMR where we do this: https://github.com/eclipse/omr/blob/b9d1250dea50a28faf03024394ff36ed99c4c2d1/compiler/z/codegen/BinaryEvaluator.cpp#L1838-L1840

Two questions come to mind:

  1. This should be out of OMR right (especially the comment)?
  2. Should we still mask with 0x1f before doing a shift (or any other value)? I would think not.
dchopra001 commented 6 years ago

FWIW, shift instructions in Z like SLL/SLLK/SLLG only consider the first 6 bits of the register holding the shift amount. So the shift amount can only be 0-63. It doesn't say anything about only considering the first 5 bits for the 32 bit variant of the instruction (SLL/SLLK). So perhaps there's a case for masking when we are doing Integer shifts.

fjeremic commented 6 years ago

To answer your questions:

  1. Yes the comment should be removed or altered to not mention Java.
  2. I think this enters a grey area. Who consumes the API? I would guess some shift IL evaluates. I don't think we have exact specifications on how a shift should behave for a shift of more bits than the width of the operand. @0xdaryl do you might know? Most languages define this as undefined behavior or they prevent it by masking (such as Java).
ymanton commented 6 years ago

I think this enters a grey area. Who consumes the API? I would guess some shift IL evaluates. I don't think we have exact specifications on how a shift should behave for a shift of more bits than the width of the operand. @0xdaryl do you might know? Most languages define this as undefined behavior or they prevent it by masking (such as Java).

The assumption throughout our codebase is that such shifts have hardware semantics. Another assumption throughout the codebase is that we want Java semantics, which is why in various pre-instruction selection places we query the code generator to figure out if it needs normalization and if so create the appropriate trees (e.g. https://github.com/eclipse/omr/blob/b9d1250dea50a28faf03024394ff36ed99c4c2d1/compiler/optimizer/OMRSimplifierHandlers.cpp#L1536-L1568) or in the above case just mask the shift amount in the code generator.

This is a bit of a wart, I don't think you want to go with hardcoded Java semantics in OMR because other languages will want other things. There are really only three possibilities that a language would want: mask shifts to the width of the value, define shifts greater than the width to result in 0, or have them be undefined. I think what you want is for languages to be able to call out the semantics they want and for OMR to create the appropriate trees to make the hardware shift behave like the language wants. In some cases this will be a no-op because the hardware matches the language or the language doesn't care, and in other cases you'll have the appropriate semantics in the IL.

Not sure what the best way to do the above would be, but off the top of my head either have different shift IL ops for different semantics or have a flag/property on shift nodes that defines their semantics.

fjeremic commented 6 years ago

Not sure what the best way to do the above would be, but off the top of my head either have different shift IL ops for different semantics or have a flag/property on shift nodes that defines their semantics.

A third option could be to have a flag on the codegenerator itself so the IL does not have to carry this state.

dchopra001 commented 6 years ago

As mentioned in a discussion comment in #2330: https://github.com/eclipse/omr/pull/2330#discussion_r170142144

We decided not to mask the shift amount for 32 bit integer shifts for the bitPermute evaluator. The IL for which that evaluator is written also doesn't support the behaviour when a shift amount greater than the width of the value is specified.

I'm leaving a note here to be on the safe side, so that if in the future we need to go back on this decision, we can do so easily.