[MERGE #219] Memset on typed specialized values

Merge pull request #219 from Cellule:users/micfer/memop_var
Sometimes, when type specializing the original instruction is lost.
ie: `s3.var          =  LdC_A_I4       0 (0x0).i32` is type specialized to `int32`, but the dst is used as a float.
If it's the only use, then `s3.var` is removed.
Previously memset was trying to get `s3.var` from the float type specialized version.
However, it is not live anymore.

This change makes memset use directly the value the `StElemI_A` is using (ie: the float typed specialized register)
And will call ToVar on it.
A better fix would be to try to use the var version first if it is available, but might introduce unexpected bugs.

I also added several test cases to cover this issue and more, along with some tests with simds.

lib/Backend/FlowGraph.h

@@ -619,9 +619,9 @@ public:
     struct MemSetCandidate : public MemOpCandidate
     {
         BailoutConstantValue constant;
-        StackSym* varSym;
+        StackSym* srcSym;
 
-        MemSetCandidate() : MemOpCandidate(MemOpCandidate::MEMSET), varSym(nullptr) {}
+        MemSetCandidate() : MemOpCandidate(MemOpCandidate::MEMSET), srcSym(nullptr) {}
     };
 
     struct MemCopyCandidate : public MemOpCandidate

lib/Backend/GlobOpt.cpp

@@ -4248,23 +4248,14 @@ GlobOpt::CollectMemsetStElementI(IR::Instr *instr, Loop *loop)
     SymID baseSymID = GetVarSymID(baseOp->GetStackSym());
 
     IR::Opnd *srcDef = instr->GetSrc1();
-    StackSym *varSym = nullptr;
+    StackSym *srcSym = nullptr;
     if (srcDef->IsRegOpnd())
     {
         IR::RegOpnd* opnd = srcDef->AsRegOpnd();
         if (this->OptIsInvariant(opnd, this->currentBlock, loop, this->FindValue(opnd->m_sym), true, true))
         {
-            StackSym* sym = opnd->GetStackSym();
-            if (sym->GetType() != TyVar)
-            {
-                varSym = sym->GetVarEquivSym(instr->m_func);
-            }
-            else
-            {
-                varSym = sym;
-            }
+            srcSym = opnd->GetStackSym();
         }
-
     }
 
     BailoutConstantValue constant = {TyIllegal, 0};
@@ -4280,7 +4271,7 @@ GlobOpt::CollectMemsetStElementI(IR::Instr *instr, Loop *loop)
     {
         constant.InitVarConstValue(srcDef->AsAddrOpnd()->m_address);
     }
-    else if(!varSym)
+    else if(!srcSym)
     {
         TRACE_MEMOP_PHASE_VERBOSE(MemSet, loop, instr, L"Source is not an invariant");
         return false;
@@ -4297,7 +4288,7 @@ GlobOpt::CollectMemsetStElementI(IR::Instr *instr, Loop *loop)
     memsetInfo->base = baseSymID;
     memsetInfo->index = inductionSymID;
     memsetInfo->constant = constant;
-    memsetInfo->varSym = varSym;
+    memsetInfo->srcSym = srcSym;
     memsetInfo->count = 1;
     memsetInfo->bIndexAlreadyChanged = isIndexPreIncr;
     loop->memOpInfo->candidates->Prepend(memsetInfo);
@@ -20821,10 +20812,9 @@ GlobOpt::EmitMemop(Loop * loop, LoopCount *loopCount, const MemOpEmitData* emitD
     {
         MemSetEmitData* data = (MemSetEmitData*)emitData;
         const Loop::MemSetCandidate* candidate = data->candidate->AsMemSet();
-        if (candidate->varSym)
+        if (candidate->srcSym)
         {
-            Assert(candidate->varSym->GetType() == TyVar);
-            IR::RegOpnd* regSrc = IR::RegOpnd::New(candidate->varSym, TyVar, func);
+            IR::RegOpnd* regSrc = IR::RegOpnd::New(candidate->srcSym, candidate->srcSym->GetType(), func);
             regSrc->SetIsJITOptimizedReg(true);
             src1 = regSrc;
         }
@@ -20877,9 +20867,9 @@ GlobOpt::EmitMemop(Loop * loop, LoopCount *loopCount, const MemOpEmitData* emitD
             const Loop::MemSetCandidate* candidate = emitData->candidate->AsMemSet();
             const int constBufSize = 32;
             wchar_t constBuf[constBufSize];
-            if (candidate->varSym)
+            if (candidate->srcSym)
             {
-                _snwprintf_s(constBuf, constBufSize, L"s%u", candidate->varSym->m_id);
+                _snwprintf_s(constBuf, constBufSize, L"s%u", candidate->srcSym->m_id);
             }
             else
             {

lib/Backend/Lower.cpp

@@ -1508,7 +1508,7 @@ Lowerer::LowerRange(IR::Instr *instrStart, IR::Instr *instrEnd, bool defaultDoFa
         case Js::OpCode::Memset:
         case Js::OpCode::Memcopy:
         {
-            LowerMemOp(instr);
+            instrPrev = LowerMemOp(instr);
             break;
         }
 
@@ -8379,7 +8379,7 @@ Lowerer::LowerLdArrViewElem(IR::Instr * instr)
     return instrPrev;
 }
 
-void
+IR::Instr *
 Lowerer::LowerMemset(IR::Instr * instr, IR::RegOpnd * helperRet)
 {
     IR::Opnd * dst = instr->UnlinkDst();
@@ -8396,7 +8396,14 @@ Lowerer::LowerMemset(IR::Instr * instr, IR::RegOpnd * helperRet)
     Assert(indexOpnd);
 
     IR::JnHelperMethod helperMethod = IR::HelperOp_Memset;
-
+    IR::Instr *instrPrev = nullptr;
+    if (src1->IsRegOpnd() && !src1->IsVar())
+    {
+        IR::RegOpnd* varOpnd = IR::RegOpnd::New(TyVar, instr->m_func);
+        instrPrev = IR::Instr::New(Js::OpCode::ToVar, varOpnd, src1, instr->m_func);
+        instr->InsertBefore(instrPrev);
+        src1 = varOpnd;
+    }
     instr->SetDst(helperRet);
     LoadScriptContext(instr);
     m_lowererMD.LoadHelperArgument(instr, sizeOpnd);
@@ -8405,9 +8412,11 @@ Lowerer::LowerMemset(IR::Instr * instr, IR::RegOpnd * helperRet)
     m_lowererMD.LoadHelperArgument(instr, baseOpnd);
     m_lowererMD.ChangeToHelperCall(instr, helperMethod);
     dst->Free(m_func);
+    
+    return instrPrev;
 }
 
-void
+IR::Instr *
 Lowerer::LowerMemcopy(IR::Instr * instr, IR::RegOpnd * helperRet)
 {
     IR::Opnd * dst = instr->UnlinkDst();
@@ -8442,6 +8451,8 @@ Lowerer::LowerMemcopy(IR::Instr * instr, IR::RegOpnd * helperRet)
     m_lowererMD.ChangeToHelperCall(instr, helperMethod);
     dst->Free(m_func);
     src->Free(m_func);
+
+    return nullptr;
 }
 
 IR::Instr *
@@ -8504,13 +8515,19 @@ Lowerer::LowerMemOp(IR::Instr * instr)
         instr->ClearBailOutInfo();
     }
 
+    IR::Instr* newInstrPrev = nullptr;
     if (instr->m_opcode == Js::OpCode::Memset)
     {
-        LowerMemset(instr, helperRet);
+        newInstrPrev = LowerMemset(instr, helperRet);
     }
     else if (instr->m_opcode == Js::OpCode::Memcopy)
     {
-        LowerMemcopy(instr, helperRet);
+        newInstrPrev = LowerMemcopy(instr, helperRet);
+    }
+
+    if (newInstrPrev != nullptr)
+    {
+        instrPrev = newInstrPrev;
     }
     return instrPrev;
 }

lib/Backend/Lower.h

@@ -158,8 +158,8 @@ private:
     void            LowerLdLen(IR::Instr *const instr, const bool isHelper);
 
     IR::Instr *     LowerMemOp(IR::Instr * instr);
-    void            LowerMemset(IR::Instr * instr, IR::RegOpnd * helperRet);
-    void            LowerMemcopy(IR::Instr * instr, IR::RegOpnd * helperRet);
+    IR::Instr *     LowerMemset(IR::Instr * instr, IR::RegOpnd * helperRet);
+    IR::Instr *     LowerMemcopy(IR::Instr * instr, IR::RegOpnd * helperRet);
 
     IR::Instr *     LowerLdArrViewElem(IR::Instr * instr);
     IR::Instr *     LowerStArrViewElem(IR::Instr * instr);

test/Array/memset_invariant.js

@@ -3,116 +3,24 @@
 // Licensed under the MIT license. See LICENSE.txt file in the project root for full license information.
 //-------------------------------------------------------------------------------------------------------
 
-function* makeValueGen(a, b) {
-  // return a for profiling
-  yield a;
-  // return b to bailout
-  yield b;
-  // return b again to compare with non jit function
-  yield b;
-}
-
-function* makeStartGen(a, b) {
-  yield 0; // for interpreter
-  yield 32; // for jitted version
-  yield 32; // for jitted version
-}
-
-function makeTest(name, config) {
-  const f1 = `function ${name}(arr) {
-    for(var i = -5; i < 15; ++i) {arr[i] = ${config.v1};}
-    return arr;
-  }`;
-  const f2 = customName => `function ${customName}P(arr, v) {
-    for(var i = 1; i < 8; ++i) {arr[i] = v;}
-    return arr;
-  }`;
-  const f3 = `function ${name}V(arr) {
-    const v = ${config.v1};
-    for(var i = -2; i < 17; ++i) {arr[i] = v;}
-    return arr;
-  }`;
-  const f4 = customName => `function ${customName}Z(arr, start) {
-    const v = ${config.v1};
-    for(var i = start; i < 5; ++i) {arr[i] = v;}
-    return arr;
-  }`;
-
-  const extraTests = (config.wrongTypes || []).map((wrongType, i) => {
-    const difValue = {f: f2(`${name}W${i}`), compare: f2(`${name}WC${i}`)};
-    const genValue = makeValueGen(config.v1, wrongType);
-    Reflect.defineProperty(difValue, "v", {
-      get: () => genValue.next().value
-    });
-    return difValue;
-  });
-
-  const negativeLengthTest = {f: f4(name), compare: f4(`${name}C`), newForCompare: true};
-  const genIndex = makeStartGen();
-  Reflect.defineProperty(negativeLengthTest, "v", {
-    get: () => genIndex.next().value
-  });
-
-  const tests = [
-    {f: f1},
-    {f: f2(name), v: config.v2 !== undefined ? config.v2 : config.v1},
-    {f: f3},
-    negativeLengthTest
-  ].concat(extraTests);
-
-  const convertTest = function(fnText) {
-    var fn;
-    eval(`fn = ${fnText}`);
-    return fn;
-  };
-  for(const t of tests) {
-    t.f = convertTest(t.f);
-    t.compare = t.compare && convertTest(t.compare);
-  }
-  return tests;
-}
+this.WScript.LoadScriptFile(".\\memset_tester.js");
 
 const allTypes = [0, 1.5, undefined, null, 9223372036854775807, "string", {a: null, b: "b"}];
+
 const tests = [
-  {name: "memsetUndefined", v1: undefined, wrongTypes: allTypes},
-  {name: "memsetNull", v1: null, wrongTypes: allTypes},
-  {name: "memsetFloat", v1: 3.14, v2: -87.684, wrongTypes: allTypes},
-  {name: "memsetNumber", v1: 9223372036854775807, v2: -987654987654987, wrongTypes: allTypes},
-  {name: "memsetBoolean", v1: true, v2: false, wrongTypes: allTypes},
-  {name: "memsetString", v1: "\"thatString\"", v2: "`A template string`", wrongTypes: allTypes},
-  {name: "memsetObject", v1: "{test: 1}", v2: [1, 2, 3], wrongTypes: allTypes},
+  {name: "memsetUndefined", stringValue: undefined},
+  {name: "memsetNull", stringValue: null},
+  {name: "memsetInt", stringValue: 0, v2: 1 << 30},
+  {name: "memsetFloat", stringValue: 3.14, v2: -87.684},
+  {name: "memsetNumber", stringValue: 9223372036854775807, v2: -987654987654987},
+  {name: "memsetBoolean", stringValue: true, v2: false},
+  {name: "memsetString", stringValue: "\"thatString\"", v2: "`A template string`"},
+  {name: "memsetObject", stringValue: "{test: 1}", v2: [1, 2, 3]},
 ];
 
 const types = "Int8Array Uint8Array Int16Array Uint16Array Int32Array Uint32Array Float32Array Float64Array Array".split(" ");
-const global = this;
 
-let passed = true;
-for(const test of tests) {
-  for(const t of types) {
-    const fns = makeTest(`${test.name}${t}`, test);
-    for(const detail of fns) {
-      const fn = detail.f;
-      let a1 = fn(new global[t](10), detail.v);
-      const a2 = fn(new global[t](10), detail.v);
-      if(detail.compare) {
-        // the optimized version ran with a different value. Run again with a clean function to compare=
-        a1 = detail.compare(detail.newForCompare ? new global[t](10) : a1, detail.v);
-      }
-      if(a1.length !== a2.length) {
-        passed = false;
-        print(`${fn.name} (${t}) didn't return arrays with same length`);
-        continue;
-      }
-      for(let i = 0; i < a1.length; ++i) {
-        if(a1[i] !== a2[i] && !(isNaN(a1[i]) && isNaN(a2[i]))) {
-          passed = false;
-          print(`${fn.name} (${t}): a1[${i}](${a1[i]}) != a2[${i}](${a2[i]})`);
-          break;
-        }
-      }
-    }
-  }
-}
+let passed = RunMemsetTest(tests, types, allTypes);
 
 function memsetSymbol() {const s = Symbol(); const arr = new Array(10); for(let i = 0; i < 10; ++i) {arr[i] = s;} return arr;}
 function memsetSymbolV(v) {const arr = new Array(10); for(let i = 0; i < 10; ++i) {arr[i] = v;} return arr;}

test/Array/memset_simd.js

@@ -0,0 +1,49 @@
+//-------------------------------------------------------------------------------------------------------
+// Copyright (C) Microsoft. All rights reserved.
+// Licensed under the MIT license. See LICENSE.txt file in the project root for full license information.
+//-------------------------------------------------------------------------------------------------------
+
+this.WScript.LoadScriptFile("memset_tester.js");
+
+const simdRegex = /^\w+(\d+)?x(\d+)$/;
+const allSimdTypes = Object.getOwnPropertyNames(SIMD)
+  // just to make sure
+  .filter(simdType => simdRegex.test(simdType))
+  .map(simdType => {
+    const result = simdRegex.exec(simdType);
+    const nLanes = parseInt(result[2]);
+    const simdInfo = {
+      makeSimd() {
+        const args = new Array(nLanes);
+        for(let i = 0; i < nLanes; ++i) {
+          args[i] = Math.random() * (1 << 62);
+        }
+        return SIMD[simdType](...args);
+      },
+      makeStringValue() {
+        const args = new Array(nLanes);
+        for(let i = 0; i < nLanes; ++i) {
+          args[i] = Math.random() * (1 << 62);
+        }
+        return `SIMD.${simdType}(${args.join(",")})`;
+      },
+      nLanes,
+      simdType
+    };
+    return simdInfo;
+  });
+
+const allTypes = [0, 1.5, undefined, null, 9223372036854775807, "string", {a: null, b: "b"}];
+const tests = allSimdTypes.map(simdInfo => {
+  return {
+    name: `memset${simdInfo.simdType}`,
+    stringValue: simdInfo.makeStringValue(),
+    v2: simdInfo.makeSimd()
+  };
+});
+
+const types = "Array".split(" ");
+
+let passed = RunMemsetTest(tests, types, allTypes);
+
+print(passed ? "PASSED" : "FAILED");

test/Array/memset_tester.js

@@ -0,0 +1,124 @@
+//-------------------------------------------------------------------------------------------------------
+// Copyright (C) Microsoft. All rights reserved.
+// Licensed under the MIT license. See LICENSE.txt file in the project root for full license information.
+//-------------------------------------------------------------------------------------------------------
+
+function* makeValueGen(a, b) {
+  // return a for profiling
+  yield a;
+  // return b to bailout
+  yield b;
+  // return b again to compare with non jit function
+  yield b;
+}
+
+function* makeStartGen() {
+  yield 0; // for interpreter
+  yield 32; // for jitted version
+  yield 32; // for jitted version
+}
+
+const global = this;
+function RunMemsetTest(tests, arrayTypes, allTypes) {
+  function makeTest(name, config) {
+    const f1 = `function ${name}(arr) {
+      for(var i = 0; i < 15; ++i) {arr[i] = ${config.stringValue};}
+      return arr;
+    }`;
+    const f2 = customName => `function ${customName}P(arr, v) {
+      for(var i = 1; i < 8; ++i) {arr[i] = v;}
+      return arr;
+    }`;
+    const f3 = `function ${name}V(arr) {
+      const v = ${config.stringValue};
+      for(var i = -2; i < 17; ++i) {arr[i] = v;}
+      return arr;
+    }`;
+    const f4 = customName => `function ${customName}Z(arr, start) {
+      const v = ${config.stringValue};
+      for(var i = start; i < 5; ++i) {arr[i] = v;}
+      return arr;
+    }`;
+
+    const extraTests = allTypes.map((diffType, i) => {
+      const difValue = {f: f2(`${name}W${i}`), compare: f2(`${name}WC${i}`)};
+      const genValue = makeValueGen(config.stringValue, diffType);
+      Reflect.defineProperty(difValue, "v", {
+        get: () => genValue.next().value
+      });
+      return difValue;
+    });
+
+    const negativeLengthTest = {f: f4(name), compare: f4(`${name}C`), newForCompare: true};
+    const genIndex = makeStartGen();
+    Reflect.defineProperty(negativeLengthTest, "v", {
+      get: () => genIndex.next().value
+    });
+
+    const tests = [
+      {f: f1},
+      {f: f2(name), v: config.v2 !== undefined ? config.v2 : config.stringValue},
+      {f: f3},
+      negativeLengthTest
+    ].concat(extraTests);
+
+    const convertTest = function(fnText) {
+      var fn;
+      eval(`fn = ${fnText}`);
+      return fn;
+    };
+    for(const t of tests) {
+      t.f = convertTest(t.f);
+      t.compare = t.compare && convertTest(t.compare);
+    }
+    return tests;
+  }
+
+
+  let passed = true;
+  for(const test of tests) {
+    for(const t of arrayTypes) {
+      const set1 = makeTest(`${test.name}${t}`, test);
+      const testSets = [{
+        set: set1,
+        getArray() {return new global[t](10);}
+      }].concat(allTypes.map((diffType, iType) => {
+        return {
+          set: makeTest(`${test.name}${t}${iType}`, test).slice(0, 1),
+          getArray() {const arr = new global[t](10); for(let i = 0; i < 10; ++i) arr[i] = diffType; return arr;}
+        };
+      }));
+      for(const testSet of testSets) {
+        for(const detail of testSet.set) {
+          const fn = detail.f;
+          try {
+            let a1 = fn(testSet.getArray(), detail.v);
+            const a2 = fn(testSet.getArray(), detail.v);
+            if(detail.compare) {
+              // the optimized version ran with a different value. Run again with a clean function to compare=
+              a1 = detail.compare(detail.newForCompare ? testSet.getArray() : a1, detail.v);
+            }
+            if(a1.length !== a2.length) {
+              passed = false;
+              print(`${fn.name} (${t}) didn't return arrays with same length`);
+              continue;
+            }
+            for(let i = 0; i < a1.length; ++i) {
+              if(a1[i] !== a2[i] && !(isNaN(a1[i]) && isNaN(a2[i]))) {
+                passed = false;
+                print(`${fn.name} (${t}): a1[${i}](${a1[i]}) != a2[${i}](${a2[i]})`);
+                break;
+              }
+            }
+          } catch(e) {
+            if (e.message.indexOf("Number Expected") !== -1) {
+              print(e.message);
+              passed = false;
+            }
+          }
+        }
+      }
+    }
+  }
+  return passed;
+}

test/Array/rlexe.xml

@@ -631,6 +631,12 @@
         <compile-flags>-mic:1 -off:simplejit -mmoc:0 -off:JITLoopBody</compile-flags>
      </default>
   </test>
+  <test>
+     <default>
+        <files>memset_simd.js</files>
+        <compile-flags>-mic:1 -off:simplejit -mmoc:0 -off:JITLoopBody -simdjs -simd128typespec</compile-flags>
+     </default>
+  </test>
   <test>
      <default>
         <files>memset2.js</files>