Add bitpermute opcode #2156

ncough · 2017-12-18T21:53:41Z

This change adds a new opcode, bitpermute, with 8, 16, 32 and 64 bit variants.

The opcode takes a value and an array of bit indices. It then constructs a new value, of the same width as the original, by extracting bits at these indices in the original value and placing them in the lower bits of the result, corresponding to their position in the bit index array. Unspecified bits are zeroed. This calculation can be expressed as:

result = 0
for (x = 0; x < length; ++x)
  result |= ((value >> indices[x]) & 1) << x
return result

The node has three children:

the value, which should be an 8, 16, 32 or 64 bit integer, matching the opcode's result
the address of the byte array
the array's length, as a 32 bit integer

The opcode is specified using shifts, rather than bit numbering, to remain consistent across platforms. Similar to the shift operations, the behaviour for bit indices greater the width of the value is undefined. This affects both indices in the byte array greater than the width and if the number of specified bits, the array length, exceeds the width of the result.

This change includes an X86 implementation, which consists of a series of bit tests and sets. The current implementation has an optimization for constant length arrays, a common case under JProfiling.

ncough · 2017-12-18T21:54:14Z

@0dvictor Could you please review the X86 implementation?

ncough · 2017-12-18T21:58:43Z

@fjeremic You may be interested in a Z implementation of this operation to improve JProfiling performance.

fjeremic · 2017-12-18T22:24:37Z

compiler/il/ILProps.hpp

@@ -228,8 +228,7 @@ namespace ILProp2
      ZeroExtension                = 0x01000000,
      SignExtension                = 0x02000000,
      ByteSwap                     = 0x04000000,
-      // Available                 = 0x08000000,
-      // Available                 = 0x10000000,
+      BitPermute                   = 0x08000000,


We seem to have deleted the 0x10000000 available line. We should leave that line in so others can use it. Otherwise a glossing eye may overlook that we jump from 0x08000000 to 0x20000000

Also why do we need this flag exactly? Doesn't the IL imply this already?

Yes, this was an accidental removal carried over from another set of changes.

The flag was added in an attempt to mirror ByteSwap's changes as a template for adding an opcode. Neither of the opcodes seem to make use of if. I'll remove BitPermute.

Leonardo2718 · 2017-12-19T01:13:37Z

Any chance of getting 8 and 16-bit variants? Maybe not as part of this PR, but could you open an issue about it? 🙂

ncough · 2017-12-19T15:38:39Z

8-bit and 16-bit variants have been added.

andrewcraik · 2017-12-19T20:50:53Z

looks reasonable to me

0dvictor · 2017-12-19T20:53:00Z

Looks good to me.

ncough · 2017-12-19T21:54:49Z

I have added TRIL tests for the new opcodes, which should help in keeping behaviour consistent across the platforms.

0xdaryl · 2017-12-20T00:09:39Z

Huge thumbs up for the Tril tests. Great addition @ncough !

fjeremic · 2017-12-20T00:54:50Z

Huge thumbs up for the Tril tests. Great addition @ncough !

This should set a precedent for any new IL to be added to OMR. IMO all new IL must come with a fairly exhaustive Tril test suite and full documentation. Awesome work @ncough! Thanks for your contribution.

Leonardo2718

Tests look great! Awesome job @ncough 👍

vijaysun-omr · 2017-12-20T01:54:08Z

genie-omr build all

0xdaryl

Minor suggestions only.

0xdaryl · 2017-12-20T00:11:54Z

compiler/codegen/OMRCodeGenerator.cpp

@@ -1023,6 +1023,17 @@ OMR::CodeGenerator::getSupportsIbyteswap()
   return false;
   }

+/**
+ * Query whether ibitpermute and lbitpermute are supported


Should this be: "whether [bsil]bitpermute IL opcodes are supported" ? It covers more than just i and l.

Yes, this was missed with the addition of the other variants. Thanks for finding it.

0xdaryl · 2017-12-20T00:38:35Z

compiler/x/codegen/OMRTreeEvaluator.cpp

+         if (x < 8)
+            generateRegRegInstruction(OR1RegReg, node, resultReg, tmpReg, cg);
+         else
+            generateRegRegInstruction(ORRegReg(nodeIs64Bit), node, resultReg, tmpReg, cg);


You can save some DLL code size here by generating one call with differing opcodes, something like:

TR_X86OpCodes op = (x < 8) ? OR1RegReg : ORRegReg(nodeIs64Bit); generateRegRegInstruction(op, node, resultReg, tmpReg, cg);

Good point. I have followed your suggestion and improved it.

0xdaryl · 2017-12-20T12:58:44Z

genie-omr build all

This change adds a new opcode: bitpermute. The instruction takes an original value and an array of bit indices. It then constructs a new value by extracting bits at these indices in the original value and placing them in the lower bits of the new. Signed-off-by: Nicholas Coughlin <cnic@ca.ibm.com>

ncough · 2017-12-20T13:36:09Z

There was an issue with the disable used to limit the Tril tests according to platform, as its currently only supported on X86. This has been corrected and other platforms should be excluded. It should be noted that this does not use cg->getSupportsBitPermute() to test for support, as its not immediately obvious how to access the codegen.

vijaysun-omr · 2017-12-20T13:57:41Z

genie-omr build all

vijaysun-omr · 2017-12-20T14:33:46Z

Test have passed. Merging.

fjeremic · 2017-12-20T16:29:06Z

There was an issue with the disable used to limit the Tril tests according to platform, as its currently only supported on X86. This has been corrected and other platforms should be excluded. It should be noted that this does not use cg->getSupportsBitPermute() to test for support, as its not immediately obvious how to access the codegen.

@ncough just to clarify this means running comptest on non-x86 platforms will result in the "Opcode not implemented" asserts once the BitPermute.cpp tests run?

ncough · 2017-12-20T16:38:12Z

@fjeremic The BitPermute.cpp tests won't be instantiated on non-x86 platforms, as they are guarded by TR_TARGET_X86, avoiding any asserts. As support for the instruction is introduced on other platforms, this test will be have to be updated.

fjeremic · 2017-12-20T16:43:07Z

@fjeremic The BitPermute.cpp tests won't be instantiated on non-x86 platforms, as they are guarded by TR_TARGET_X86, avoiding any asserts. As support for the instruction is introduced on other platforms, this test will be have to be updated.

@dchopra001 FYI. You can use this as an example for Z on how to avoid VectorTest.cpp in #1941 until we have proper Tril support for platform determination.

ncough mentioned this pull request Dec 18, 2017

Add support for JProfiling index hash eclipse-openj9/openj9#846

Merged

fjeremic reviewed Dec 18, 2017

View reviewed changes

ncough force-pushed the jprofilingInstruction branch from 9852f34 to 7ee28df Compare December 18, 2017 22:34

vijaysun-omr self-assigned this Dec 19, 2017

0xdaryl added the comp:compiler label Dec 19, 2017

ncough force-pushed the jprofilingInstruction branch 4 times, most recently from 1458d3e to 5674d61 Compare December 19, 2017 15:34

ncough force-pushed the jprofilingInstruction branch 2 times, most recently from d23e71c to 781c616 Compare December 19, 2017 22:25

Leonardo2718 approved these changes Dec 20, 2017

View reviewed changes

vijaysun-omr approved these changes Dec 20, 2017

View reviewed changes

ncough force-pushed the jprofilingInstruction branch from 781c616 to d91cb31 Compare December 20, 2017 06:50

0xdaryl requested changes Dec 20, 2017

View reviewed changes

ncough force-pushed the jprofilingInstruction branch from d91cb31 to a2c4537 Compare December 20, 2017 12:42

0xdaryl approved these changes Dec 20, 2017

View reviewed changes

ncough force-pushed the jprofilingInstruction branch from a2c4537 to c0f38c1 Compare December 20, 2017 13:26

ncough force-pushed the jprofilingInstruction branch from c0f38c1 to d660085 Compare December 20, 2017 13:27

vijaysun-omr merged commit bff5a5a into eclipse:master Dec 20, 2017

dchopra001 mentioned this pull request Feb 20, 2018

Add the bitPermute Evaluator for Z #2330

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add bitpermute opcode #2156

Add bitpermute opcode #2156

ncough commented Dec 18, 2017 •

edited

Loading

ncough commented Dec 18, 2017

ncough commented Dec 18, 2017

fjeremic Dec 18, 2017

ncough Dec 18, 2017

Leonardo2718 commented Dec 19, 2017

ncough commented Dec 19, 2017

andrewcraik commented Dec 19, 2017

0dvictor commented Dec 19, 2017

ncough commented Dec 19, 2017

0xdaryl commented Dec 20, 2017

fjeremic commented Dec 20, 2017

Leonardo2718 left a comment

vijaysun-omr commented Dec 20, 2017

0xdaryl left a comment

0xdaryl Dec 20, 2017

ncough Dec 20, 2017

0xdaryl Dec 20, 2017

ncough Dec 20, 2017

0xdaryl commented Dec 20, 2017

ncough commented Dec 20, 2017

vijaysun-omr commented Dec 20, 2017

vijaysun-omr commented Dec 20, 2017

fjeremic commented Dec 20, 2017

ncough commented Dec 20, 2017

fjeremic commented Dec 20, 2017

Add bitpermute opcode #2156

Add bitpermute opcode #2156

Conversation

ncough commented Dec 18, 2017 • edited Loading

ncough commented Dec 18, 2017

ncough commented Dec 18, 2017

fjeremic Dec 18, 2017

Choose a reason for hiding this comment

ncough Dec 18, 2017

Choose a reason for hiding this comment

Leonardo2718 commented Dec 19, 2017

ncough commented Dec 19, 2017

andrewcraik commented Dec 19, 2017

0dvictor commented Dec 19, 2017

ncough commented Dec 19, 2017

0xdaryl commented Dec 20, 2017

fjeremic commented Dec 20, 2017

Leonardo2718 left a comment

Choose a reason for hiding this comment

vijaysun-omr commented Dec 20, 2017

0xdaryl left a comment

Choose a reason for hiding this comment

0xdaryl Dec 20, 2017

Choose a reason for hiding this comment

ncough Dec 20, 2017

Choose a reason for hiding this comment

0xdaryl Dec 20, 2017

Choose a reason for hiding this comment

ncough Dec 20, 2017

Choose a reason for hiding this comment

0xdaryl commented Dec 20, 2017

ncough commented Dec 20, 2017

vijaysun-omr commented Dec 20, 2017

vijaysun-omr commented Dec 20, 2017

fjeremic commented Dec 20, 2017

ncough commented Dec 20, 2017

fjeremic commented Dec 20, 2017

ncough commented Dec 18, 2017 •

edited

Loading