Lowerer UseWithNewType() and Legalize() counter-intuitive behavior

[nmostafa]

Some SIMD.js x64test jshost failures were related to regAlloc generating bad code if casting to TyInt8. And I had to pin TyInt8 operands to a byteable register (#549).

What we want to do is cast reg2 to 1-byte then sign-extend:

        IR::RegOpnd *reg2 = IR::RegOpnd::New(TyInt32, m_func);
        IR::RegOpnd * tmp = IR::RegOpnd::New(TyInt8, m_func);
        instr->InsertBefore(IR::Instr::New(Js::OpCode::MOV, tmp->UseWithNewType(TyInt32, m_func), reg2, m_func));

        instr->InsertBefore(IR::Instr::New(Js::OpCode::MOVSX, reg2, tmp, m_func));

The issue is that UseWithNewType uses the original RegOpnd object if it's not already used by another instruction. Since the MOV is the first use, we inadvertently changed the type of tmp in MOVSX as well. I suggest have UseWithNewType() always copy the opnd (or at least assert this is not the first use of the opnd).

We also tried this:

        IR::RegOpnd *reg2 = IR::RegOpnd::New(TyInt32, m_func);
        IR::RegOpnd * tmp = IR::RegOpnd::New(TyInt8, m_func);
        instr->InsertBefore(IR::Instr::New(Js::OpCode::MOV, tmp, reg2, m_func));
        instr->InsertBefore(IR::Instr::New(Js::OpCode::MOVSX, reg2, tmp, m_func));

The problem here is that we missed legalization of the MOV instruction, which should fix reg2 to be TyInt8. However, LowererMD::Legalize< verify=true >() doesn't assert due to the malformed code, but instead fixes it silently. The code is then valid only in debug mode, and breaks in test/release. I suggest we assert there.

template <bool verify>
void
LowererMD::Legalize(IR::Instr *const instr, bool fPostRegAlloc)
{
    switch(instr->m_opcode)
   {
    ...
        case Js::OpCode::MOV:
        {
            else if (TySize[dstType] < TySize[srcType])
            {
               // if verify, we should assert(false) here.
                instr->GetSrc1()->SetType(dst->GetType());
            }
        }
   ...
   }
...
}

[agarwal-sandeep]

@Cellule since SIMD is removed from code do you think we need to do anything for this for WASM simd?

//cc @kfarnung @dilijev

[Cellule]

I think the issue in general is not related to simd at all. I too encountered the same problem somewhat recently and I think we should probably rethink how UseWithNewType works. As suggested, we probably should rename it to CloneWithNewType and always make a copy.

As for Legalize, we should look more into that use case, we shouldn't silently fix the IR only in debug mode.