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

Prima facie valid IL opcodes are invalid depending on platform #566

Open lmaisons opened 7 years ago

lmaisons commented 7 years ago

Some opcodes in the compiler IL that appear to be universally valid for use may or may not be valid depending on the platform we're compiling on.

An example that came up recently is 'aiadd':

Thus, on 64 bit platforms, aiadd is invalid.

I believe this is an artifact of the time before we finalized the requirement that binary operations work on types of the same size (and banished signedness issues to widening operators).

mstoodle commented 7 years ago

in this particular case, I suppose we could introduce an aiuadd opcode to fill the gap in reasoning about whether to sign extend the integer operand. But that would be a bit of a weird scenario and I'm not sure I like it.

Better would be for the position you've advocated, which is that aiadd is an invalid opcode on 64-bit platforms (the opcode that should be used is aladd and there should be either an iu2l or i2l on the integer operand). That means we should have an assertion somewhere if anyone tries to (re)create such a node. Sadly, it also passes on some pain to people working with the IL :( .

Still, many things really need to know what that type is, anyway, so maybe the pain forces us to think about it properly (he suggested, squinting dubiously).

mgaudet commented 7 years ago

Another option would be to replace aiadd and aladd with a 'typeless' aadd, that takes either a Int32 or Int64 argument depending on address width.

All of this is evidence we also need a much better IL verification story.

Leonardo2718 commented 7 years ago

@mstoodle Actually, I believe there already exists a aiuadd.

I tend to prefer @mgaudet option as it's one less thing to think about when generating IL. Otherwise, one has to think about which opcode to generate and whether a iu2l or i2l is needed.

I also agree that IL verification definitely needs to be able to catch these kinds of errors.

mstoodle commented 7 years ago

The problem with the "generic" aadd opcode is that you're basically punting the complexity into the optimizer that's reasoning about what that thing actually means based on the platform you're running on.

I'm not sure that's the right trade-off compared to: figure out what you really mean and provide the proper input to the compiler (once).

We have an IL verification story and it has worked for a bunch of common problems. I agree that it's not perfect and could be made better. IMO improvements are very welcome so long as the verification doesn't impact production compile time (unless you can make an argument that the cost is worth it to find mistakes that are made often).

andrewcraik commented 7 years ago

I am much more in favour of a solution where we add logic to the verification to check that aiadd and aladd are only used on the correct platforms. The aadd opcode would be much harder to handle in the optimizer. We are also moving away from signed-ness on arithmetic operations (eg iuadd should be replaced with iadd since they are bit equivalent) so the verification of correct use of aiadd and aladd seems to be the nicest middle ground IMO.

0xdaryl commented 7 years ago

I believe the summary of the work needed to be done for this issue is to ensure the ILValidator is active for OMR projects and handles the case where aiadd is used on 64-bit platforms.

Any problems discovered by the ILValidator detecting these invalid patterns should result in new issues being created.