review feedback

Author: kunalspathak

src/coreclr/jit/gentree.cpp

@@ -18008,11 +18008,7 @@ bool GenTree::canBeContained() const
     }
     else if (OperIsHWIntrinsic() && !isContainableHWIntrinsic())
     {
-#ifdef TARGET_XARCH
-        return isEvexEmbeddedMaskingCompatibleHWIntrinsic();
-#elif TARGET_ARM64
-        return HWIntrinsicInfo::IsEmbeddedMaskedOperation(AsHWIntrinsic()->GetHWIntrinsicId());
-#endif
+        return isEmbeddedMaskingCompatibleHWIntrinsic();
     }
 
     return true;
@@ -19909,24 +19905,25 @@ bool GenTree::isEvexCompatibleHWIntrinsic() const
 }
 
 //------------------------------------------------------------------------
-// isEvexEmbeddedMaskingCompatibleHWIntrinsic: Checks if the intrinsic is compatible
+// isEmbeddedMaskingCompatibleHWIntrinsic : Checks if the intrinsic is compatible
 // with the EVEX embedded masking form for its intended lowering instruction.
 //
 // Return Value:
 // true if the intrisic node lowering instruction has an EVEX embedded masking
 //
-bool GenTree::isEvexEmbeddedMaskingCompatibleHWIntrinsic() const
+bool GenTree::isEmbeddedMaskingCompatibleHWIntrinsic() const
 {
-#if defined(TARGET_XARCH)
     if (OperIsHWIntrinsic())
     {
+#if defined(TARGET_XARCH)
         // TODO-AVX512F-CQ: Expand this to the full set of APIs and make it table driven
         // using IsEmbMaskingCompatible. For now, however, limit it to some explicit ids
         // for prototyping purposes.
         return (AsHWIntrinsic()->GetHWIntrinsicId() == NI_AVX512F_Add);
+#elif defined(TARGET_ARM64)
+        return HWIntrinsicInfo::IsEmbeddedMaskedOperation(AsHWIntrinsic()->GetHWIntrinsicId());
+#endif
     }
-#endif // TARGET_XARCH
-
     return false;
 }
 

src/coreclr/jit/gentree.h

@@ -1465,7 +1465,7 @@ struct GenTree
     bool isContainableHWIntrinsic() const;
     bool isRMWHWIntrinsic(Compiler* comp);
     bool isEvexCompatibleHWIntrinsic() const;
-    bool isEvexEmbeddedMaskingCompatibleHWIntrinsic() const;
+    bool isEmbeddedMaskingCompatibleHWIntrinsic() const;
 #else
     bool isCommutativeHWIntrinsic() const
     {
@@ -1487,7 +1487,7 @@ struct GenTree
         return false;
     }
 
-    bool isEvexEmbeddedMaskingCompatibleHWIntrinsic() const
+    bool isEmbeddedMaskingCompatibleHWIntrinsic() const
     {
         return false;
     }

src/coreclr/jit/hwintrinsiccodegenarm64.cpp

@@ -438,6 +438,7 @@ void CodeGen::genHWIntrinsic(GenTreeHWIntrinsic* node)
             switch (intrinOp2.numOperands)
             {
                 case 1:
+                    assert(!instrIsRMW);
                     GetEmitter()->emitIns_R_R_R(insOp2, emitSize, targetReg, maskReg, instrOp1Reg, opt);
                     break;
 
@@ -449,11 +450,14 @@ void CodeGen::genHWIntrinsic(GenTreeHWIntrinsic* node)
                         assert(targetReg != instrOp1Reg);
                         assert(targetReg != instrOp2Reg);
 
-                        if (!intrin.op3->IsVectorZero())
+                        if (!intrin.op3->IsVectorZero() && (instrOp1Reg != falseReg))
                         {
                             // If the `falseValue` of conditional-select is non-zero, then we start with that in the
                             // destination followed by `movprfx` (see below) of the relevant bits of the first operand
                             // of `insOp2` based upon the value of predicate `cond`.
+
+                            // Also, if `instrOp1Reg == falseReg`, we will move the `instrOp1Reg` using `movprfx` and
+                            // so, no need to move it separately here.
                             GetEmitter()->emitIns_Mov(INS_mov, emitTypeSize(node), targetReg, falseReg,
                                                       /* canSkip */ true);
                         }

src/coreclr/jit/lowerxarch.cpp

@@ -10137,7 +10137,7 @@ void Lowering::ContainCheckHWIntrinsic(GenTreeHWIntrinsic* node)
                                 // contained and not a memory operand and know to invoke the special handling
                                 // so that the embedded masking can work as expected.
 
-                                if (op2->isEvexEmbeddedMaskingCompatibleHWIntrinsic())
+                                if (op2->isEmbeddedMaskingCompatibleHWIntrinsic())
                                 {
                                     uint32_t maskSize = genTypeSize(simdBaseType);
                                     uint32_t operSize = genTypeSize(op2->AsHWIntrinsic()->GetSimdBaseType());