Replace AddrOpnd with IntConstOpnd for 64-bit immediates #1630

meg-gupta · 2016-09-23T02:17:11Z

AddrOpnd was used with AddrOpndKindConstant for 64bit immediates, and then
legalized. Replace all such uses with IntConstOpnd, and add code in
legalizer to handle them.

meg-gupta · 2016-09-23T21:41:29Z

@rajatd , @satheeshravi Please take a look when you can, thanks!

rajatd · 2016-09-24T23:36:47Z

lib/Backend/Lower.cpp

@@ -12936,7 +12935,7 @@ Lowerer::LowerInlineeEnd(IR::Instr *instr)
    {
        // REVIEW (michhol): OOP JIT. why are we creating an addropnd with 0?


Could get rid of this review comment. What do you think @MikeHolman?

sounds good to me

In reply to: 80370964 [](ancestors = 80370964)

rajatd · 2016-09-24T23:43:13Z

lib/Backend/LowerMDShared.cpp

    instr = IR::Instr::New(Js::OpCode::SHR, opndOffset, opndOffset, IR::IntConstOpnd::New(rightShiftAmount - leftShiftAmount, TyUint8, instrLdSt->m_func, true), instrLdSt->m_func);
    instrLdSt->InsertBefore(instr);
-    instr = IR::Instr::New(Js::OpCode::AND, opndOffset, opndOffset, IR::AddrOpnd::New((void*)((__int64)(polymorphicInlineCacheSize - 1) << leftShiftAmount), IR::AddrOpndKindConstant, instrLdSt->m_func, true), instrLdSt->m_func);
+    instr = IR::Instr::New(Js::OpCode::AND, opndOffset, opndOffset, IR::IntConstOpnd::New(((__int64)(polymorphicInlineCacheSize - 1) << leftShiftAmount), TyInt64, instrLdSt->m_func, true), instrLdSt->m_func);


__int64 [](start = 92, length = 7)

I think you should be able to remove the cast and change the type to TyMachReg

rajatd · 2016-09-24T23:45:34Z

lib/Backend/LowerMDShared.cpp

        // Unconditionally set the sign bit. This will get XORd away when we remove the tag.
        // dst64 = OR 0x8000000000000000
-        insertInstr->InsertBefore(IR::Instr::New(Js::OpCode::OR, dst, dst, IR::AddrOpnd::New((void *)MachSignBit, IR::AddrOpndKindConstant, this->m_func), this->m_func));
+        insertInstr->InsertBefore(IR::Instr::New(Js::OpCode::OR, dst, dst, IR::IntConstOpnd::New(MachSignBit, TyInt64, this->m_func), this->m_func));


TyInt64 [](start = 110, length = 7)

Consider changing this to TyMachReg

rajatd · 2016-09-24T23:48:55Z

    stackLimitOpnd = IR::AddrOpnd::New((void *)(frameSize + scriptStackLimit), IR::AddrOpndKindDynamicMisc, this->m_func);

Change this one too.

Refers to: lib/Backend/i386/LowererMDArch.cpp:1727 in dd9db5c. [](commit_id = dd9db5c, deletion_comment = False)

rajatd · 2016-09-26T17:36:13Z

LGTM other than the comments.

satheeshravi · 2016-09-26T18:05:07Z

lib/Backend/Security.cpp


    instrNew = LowererMD::CreateAssign(regOpnd, opnd, instr);

    size_t cookie = (size_t)Math::Rand();


cookie [](start = 11, length = 6)

nit: Please check if TyMachReg is ok when the value is an unsigned int. I am not sure about the implications.

satheeshravi · 2016-09-26T18:37:13Z

lib/Backend/LowerMDShared.cpp

+                if (!instr->isInlineeEntryInstr)
+                {
+                    Assert(forms & L_Reg);
+                    IR::IndirOpnd * indirOpnd = instr->m_func->GetTopFunc()->GetConstantAddressIndirOpnd(intOpnd->GetValue(), IR::AddrOpndKindConstantAddress, TyMachPtr, Js::OpCode::MOV);


intOpnd->GetValue() [](start = 105, length = 19)

This is still being treated as an address inside the API. and it looks like the API is creating AddrOpnd. Should you be creating a IntOpnd in this case ?

yes, changed this to IntConstOpnd

satheeshravi · 2016-09-26T18:37:56Z

lib/Backend/LowerMDShared.cpp

+                if (!instr->isInlineeEntryInstr)
+                {
+                    Assert(forms & L_Reg);
+                    IR::IndirOpnd * indirOpnd = instr->m_func->GetTopFunc()->GetConstantAddressIndirOpnd(intOpnd->GetValue(), IR::AddrOpndKindConstantAddress, TyMachPtr, Js::OpCode::MOV);


AddrOpndKindConstantAddress [](start = 130, length = 27)

same here.

satheeshravi · 2016-09-26T18:45:32Z

                                          IR::AddrOpnd::New((Js::Var)Js::FloatTag_Value,

Change this, as well ?

Refers to: lib/Backend/amd64/LowererMDArch.cpp:2620 in dd9db5c. [](commit_id = dd9db5c, deletion_comment = False)

dilijev · 2016-09-26T21:38:43Z

@meg-gupta FYI Tab Check issues were resolved in #1636

meg-gupta · 2016-09-28T17:21:33Z

@dotnet-bot test Windows x86_debug please

meg-gupta · 2016-09-28T18:46:39Z

@dotnet-bot test Windows arm_debug
@dotnet-bot test Windows arm_release
@dotnet-bot test Windows arm_test

satheeshravi · 2016-10-03T23:36:43Z

meg-gupta · 2016-10-06T00:42:57Z

@dotnet-bot test Windows x64_release

AddrOpnd was used with AddOpndKindConstant for 64bit immediates, and then legalized. Replace all such uses with IntConstOpnd, and add code in legalizer to handle them.

…t immediates Merge pull request #1630 from meg-gupta:newchangeimmop AddrOpnd was used with AddrOpndKindConstant for 64bit immediates, and then legalized. Replace all such uses with IntConstOpnd, and add code in legalizer to handle them.

msftclas added the cla-not-required label Sep 23, 2016

rajatd reviewed Sep 24, 2016

View reviewed changes

satheeshravi reviewed Sep 26, 2016

View reviewed changes

meg-gupta force-pushed the newchangeimmop branch from dd9db5c to c4a5d80 Compare September 28, 2016 00:49

meg-gupta force-pushed the newchangeimmop branch 2 times, most recently from e748e39 to 29936de Compare October 6, 2016 00:29

Replace AddrOpnd with IntConstOpnd for 64-bit immediates

965a30a

AddrOpnd was used with AddOpndKindConstant for 64bit immediates, and then legalized. Replace all such uses with IntConstOpnd, and add code in legalizer to handle them.

meg-gupta force-pushed the newchangeimmop branch from 29936de to 965a30a Compare October 6, 2016 00:55

chakrabot merged commit 965a30a into chakra-core:master Oct 6, 2016

rajatd mentioned this pull request Nov 11, 2016

Improve performance on some post-minifier code. #1682

Closed

		@@ -12936,7 +12935,7 @@ Lowerer::LowerInlineeEnd(IR::Instr *instr)
		{
		// REVIEW (michhol): OOP JIT. why are we creating an addropnd with 0?


		instrNew = LowererMD::CreateAssign(regOpnd, opnd, instr);

		size_t cookie = (size_t)Math::Rand();

Replace AddrOpnd with IntConstOpnd for 64-bit immediates #1630

Replace AddrOpnd with IntConstOpnd for 64-bit immediates #1630

Uh oh!

Conversation

meg-gupta commented Sep 23, 2016

Uh oh!

meg-gupta commented Sep 23, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rajatd commented Sep 24, 2016

Uh oh!

rajatd commented Sep 26, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

satheeshravi commented Sep 26, 2016

Uh oh!

dilijev commented Sep 26, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

meg-gupta commented Sep 28, 2016

Uh oh!

meg-gupta commented Sep 28, 2016

Uh oh!

satheeshravi commented Oct 3, 2016

Uh oh!

meg-gupta commented Oct 6, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

dilijev commented Sep 26, 2016 •

edited

Loading