Provide support for LZCNT and TZCNT in X86Ops.ins #6978

VidushiGit · 2023-05-05T10:46:26Z

No description provided.

github-actions

Thank you for supporting the project, and congratulations on your first contribution! A project committer will shortly review your contribution. In the mean time, if you haven't had a chance please skim over the contribution guidelines which all pull requests must adhere to. If the ECA pull request check fails, have a look at the instructions for signing the ECA in the legal considerations section.

If you run into any problems our community will be happy to assist you in any way we can. There are a number of recommended ways to interact with the community. We encourage you to ask questions, or drop by to say hello.

0xdaryl · 2023-05-05T11:46:56Z

@BradleyWood : please review

0xdaryl · 2023-05-05T11:49:01Z

Thanks for the contribution. Could you add encodings for the other register sizes as well please? i.e., 16, 32, and 64-bit

BradleyWood · 2023-05-05T13:54:31Z

Can you add binary encoding tests like the following?

INSTANTIATE_TEST_CASE_P(GeneralPurposeRegRegTest, XRegRegEncEncodingTest, ::testing::ValuesIn(*TRTest::MakeVector<std::tuple<TR::InstOpCode::Mnemonic, TR::RealRegister::RegNum, TR::RealRegister::RegNum, OMR::X86::Encoding, TRTest::BinaryInstruction>>( std::make_tuple(TR::InstOpCode::LZCNTRegReg, TR::RealRegister::eax, TR::RealRegister::ecx, OMR::X86::Default, "<binary encoding>"), std::make_tuple(TR::InstOpCode::TZCNTRegReg, TR::RealRegister::ecx, TR::RealRegister::eax, OMR::X86::Default, "<binary encoding>") )));

Edit: You'll have to fix the formatting above

VidushiGit · 2023-05-08T10:57:48Z

Thanks for the contribution. Could you add encodings for the other register sizes as well please? i.e., 16, 32, and 64-bit

Yes, I will add support for the 16 and 32- bit also. Current implementation is only supporting 64 bits. Thanks

VidushiGit · 2023-05-16T08:23:17Z

Hi Bradley,
I have added the support for 16 and 32-bit registers for the instructions.

Also I have added the test cases for the instructions as suggested. Currently I have added EAX,ECX register pair(in the same order) for testing. Please let us know if we need some other test cases added.

BradleyWood · 2023-05-16T17:41:39Z

fvtest/compilerunittest/x/BinaryEncoder.cpp

@@ -695,6 +695,14 @@ INSTANTIATE_TEST_CASE_P(AVX512MaskRegSimdEVEX512Test, XRegRegEncEncodingTest, ::
    std::make_tuple(TR::InstOpCode::VPMOVB2MRegReg,    TR::RealRegister::k2, TR::RealRegister::xmm5,   OMR::X86::EVEX_L128,  "62f27e0829d5")
 )));

+INSTANTIATE_TEST_CASE_P(GeneralPurposeRegRegTest, XRegRegEncEncodingTest, ::testing::ValuesIn(*TRTest::MakeVector<std::tuple<TR::InstOpCode::Mnemonic, TR::RealRegister::RegNum, TR::RealRegister::RegNum, OMR::X86::Encoding, TRTest::BinaryInstruction>>(
+    std::make_tuple(TR::InstOpCode::LZCNT4RegReg,   TR::RealRegister::eax,  TR::RealRegister::ecx, OMR::X86::Default, "f30fbdc1"),
+    std::make_tuple(TR::InstOpCode::LZCNT8RegReg,   TR::RealRegister::eax,  TR::RealRegister::ecx, OMR::X86::Default, "f34c0fbdc1"),


f34c0fbdc1 equates to lzcnt r8,rcx which does not match the specified registers

BradleyWood · 2023-05-16T17:41:45Z

fvtest/compilerunittest/x/BinaryEncoder.cpp

+    std::make_tuple(TR::InstOpCode::LZCNT8RegReg,   TR::RealRegister::eax,  TR::RealRegister::ecx, OMR::X86::Default, "f34c0fbdc1"),
+
+    std::make_tuple(TR::InstOpCode::TZCNT4RegReg,   TR::RealRegister::eax,  TR::RealRegister::ecx, OMR::X86::Default, "f30fbcc1"),
+    std::make_tuple(TR::InstOpCode::TZCNT8RegReg,   TR::RealRegister::eax,  TR::RealRegister::ecx, OMR::X86::Default, "f34c0fbcc1")


'f34c0fbcc1' equates to tzcnt r8,rcx which does not match the specified registers

VidushiGit · 2023-05-18T07:13:19Z

Hi,
I have updated the file : compiler/x/codegen/X86Ops.ins and added IA32OpProp1_LongTarget as a property in to define support for 64-bit target

VidushiGit · 2023-05-18T07:15:03Z

Corrected the test cases' binary encoding string

BradleyWood · 2023-05-18T15:09:52Z

jenkins build all

0xdaryl · 2023-05-23T15:19:44Z

compiler/x/codegen/X86Ops.ins

@@ -2545,6 +2545,21 @@ INSTRUCTION(LEA8RegMem, lea,
            PROPERTY0(IA32OpProp_ModifiesTarget),
            PROPERTY1(IA32OpProp1_LongSource | IA32OpProp1_LongTarget | IA32OpProp1_SourceIsMemRef),
            FEATURES(0)),
+INSTRUCTION(LZCNT2RegReg, lzcnt,
+            BINARY(VEX_L___, VEX_vNONE, PREFIX_F3, REX__, ESCAPE_0F__, 0xbd, 0, ModRM_RM__, Immediate_0),


This form should require the operand size override 0x66 prefix. Perhaps a question for @BradleyWood: in this encoding scheme, how does one encode an instruction that requires both a 0x66 and a 0xf3 prefix? Do we need a new PREFIX_66_F3 ?

I had a look over the code, as it currently stands you cannot use both prefixes.

PR #7020 was merged which provides the capability of specifying multiple prefixes. @VidushiGit, you should pick up that change and use it here.

@0xdaryl Pulled the latest OMR code changes and added the support/usage for multiple instruction prefixes

0xdaryl · 2023-05-23T23:44:41Z

compiler/x/codegen/X86Ops.ins

@@ -5144,6 +5159,21 @@ INSTRUCTION(TEST8MemReg, test,
            PROPERTY0(IA32OpProp_ModifiesOverflowFlag | IA32OpProp_ModifiesSignFlag | IA32OpProp_ModifiesZeroFlag | IA32OpProp_ModifiesParityFlag | IA32OpProp_ModifiesCarryFlag | IA32OpProp_UsesTarget),
            PROPERTY1(IA32OpProp1_LongSource | IA32OpProp1_LongTarget | IA32OpProp1_FusableCompare),
            FEATURES(0)),
+INSTRUCTION(TZCNT2RegReg, tzcnt,
+            BINARY(VEX_L___, VEX_vNONE, PREFIX_F3, REX__, ESCAPE_0F__, 0xbc, 0, ModRM_RM__, Immediate_0),


Same comment as above for the operand size prefix.

0xdaryl · 2023-05-23T23:45:29Z

fvtest/compilerunittest/x/BinaryEncoder.cpp

+    std::make_tuple(TR::InstOpCode::LZCNT8RegReg,   TR::RealRegister::eax,  TR::RealRegister::ecx, OMR::X86::Default, "f3480fbdc1"),
+
+    std::make_tuple(TR::InstOpCode::TZCNT4RegReg,   TR::RealRegister::eax,  TR::RealRegister::ecx, OMR::X86::Default, "f30fbcc1"),
+    std::make_tuple(TR::InstOpCode::TZCNT8RegReg,   TR::RealRegister::eax,  TR::RealRegister::ecx, OMR::X86::Default, "f3480fbcc1")


Can you add tests for the 16-bit versions as well?

Added tests for 16-bit version of the instruction

BradleyWood · 2023-06-08T18:12:07Z

jenkins build all

BradleyWood · 2023-06-08T18:53:14Z

MacOS failure is #6516

VidushiGit requested review from 0xdaryl and mstoodle as code owners May 5, 2023 10:46

github-actions bot added arch:x86 comp:compiler labels May 5, 2023

github-actions bot reviewed May 5, 2023

View reviewed changes

github-actions bot added the first contribution label May 5, 2023

VidushiGit force-pushed the OpsInstructions branch from d172057 to 86b469f Compare May 16, 2023 08:14

VidushiGit requested a review from charliegracie as a code owner May 16, 2023 08:14

VidushiGit force-pushed the OpsInstructions branch from 86b469f to da54bc3 Compare May 16, 2023 08:17

BradleyWood suggested changes May 16, 2023

View reviewed changes

VidushiGit force-pushed the OpsInstructions branch from da54bc3 to bbfea5c Compare May 18, 2023 06:00

BradleyWood approved these changes May 19, 2023

View reviewed changes

0xdaryl reviewed May 23, 2023

View reviewed changes

VidushiGit force-pushed the OpsInstructions branch from bbfea5c to 7186e94 Compare June 8, 2023 08:15

Provide support for LZCNT and TZCNT in X86Ops.ins

1177897

VidushiGit force-pushed the OpsInstructions branch from 7186e94 to 1177897 Compare June 8, 2023 08:54

VidushiGit requested review from BradleyWood and 0xdaryl June 8, 2023 10:42

BradleyWood approved these changes Jun 8, 2023

View reviewed changes

0xdaryl self-assigned this Jun 8, 2023

0xdaryl approved these changes Jun 8, 2023

View reviewed changes

0xdaryl merged commit 06fe1c3 into eclipse-omr:master Jun 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide support for LZCNT and TZCNT in X86Ops.ins #6978

Provide support for LZCNT and TZCNT in X86Ops.ins #6978

VidushiGit commented May 5, 2023

github-actions bot left a comment

0xdaryl commented May 5, 2023

0xdaryl commented May 5, 2023

BradleyWood commented May 5, 2023 •

edited

Loading

VidushiGit commented May 8, 2023

VidushiGit commented May 16, 2023

BradleyWood May 16, 2023

BradleyWood May 16, 2023

VidushiGit commented May 18, 2023

VidushiGit commented May 18, 2023

BradleyWood commented May 18, 2023

0xdaryl May 23, 2023

BradleyWood Jun 2, 2023

0xdaryl Jun 6, 2023

VidushiGit Jun 8, 2023

0xdaryl May 23, 2023

0xdaryl May 23, 2023

VidushiGit Jun 8, 2023

BradleyWood commented Jun 8, 2023

BradleyWood commented Jun 8, 2023

Provide support for LZCNT and TZCNT in X86Ops.ins #6978

Provide support for LZCNT and TZCNT in X86Ops.ins #6978

Conversation

VidushiGit commented May 5, 2023

github-actions bot left a comment

Choose a reason for hiding this comment

0xdaryl commented May 5, 2023

0xdaryl commented May 5, 2023

BradleyWood commented May 5, 2023 • edited Loading

VidushiGit commented May 8, 2023

VidushiGit commented May 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VidushiGit commented May 18, 2023

VidushiGit commented May 18, 2023

BradleyWood commented May 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BradleyWood commented Jun 8, 2023

BradleyWood commented Jun 8, 2023

BradleyWood commented May 5, 2023 •

edited

Loading