mul/mac src1 read suppression WA

Two issues, please check the attached document in the related document. When the mul/mach have same src1 but different region pattern with the src1 of previous instruction, a dummy instruction is need to be inserted to solve the potential read suppression bug.

Author: bcheng0127

inc/common/sku_wa_defs.h

@@ -637,4 +637,10 @@ SPDX-License-Identifier: MIT
         Wa_14020375314,
         "Workaround",
         WA_BUG_TYPE_UNKNOWN,
+        WA_BUG_PERF_IMPACT_UNKNOWN, WA_COMPONENT_UNKNOWN)
+
+       WA_DECLARE(
+        Wa_18035690555,
+        "Workaround",
+        WA_BUG_TYPE_UNKNOWN,
         WA_BUG_PERF_IMPACT_UNKNOWN, WA_COMPONENT_UNKNOWN)

visa/G4_IR.cpp

@@ -4780,6 +4780,16 @@ static bool regionHasFixedSubreg(const IR_Builder &builder, G4_Operand *opnd,
 
   G4_VarBase *base = opnd->getBase();
 
+  if (base->isAccReg()) {
+    if (opnd->isDstRegRegion()) {
+      offset = opnd->asDstRegRegion()->getSubRegOff();
+    }
+    if (opnd->isSrcRegRegion()) {
+      offset = opnd->asSrcRegRegion()->getSubRegOff();
+    }
+    return true;
+  }
+
   if (base == NULL || !base->isRegVar() ||
       !base->asRegVar()->getDeclare()->useGRF()) {
     return false;
@@ -6086,6 +6096,44 @@ bool G4_SrcRegRegion::checkGRFAlign(const IR_Builder &builder) {
   return GRF_aligned;
 }
 
+// Flat reg region: starting bit position of a channel in a register is not
+// shifted between src and dst, or limited shift of bit position e.g. big
+// int mul/mad upper dword src's or when dst-type is smaller than exec-type
+// and the dst is written to upper parts of a exec channel.
+// Return true if src region is flat region
+bool G4_SrcRegRegion::isFlatRegRegion(
+    uint8_t exChannelWidth,
+    std::function<bool(uint8_t dstStrideInBytes, uint8_t dstSubRegOffInBytes,
+                       uint8_t srcStrideInBytes, uint8_t srcSubRegOffInBytes,
+                       uint8_t exChannelWidth)> checkFlatRegRegionFunc) {
+  auto dst = inst->getDst();
+  uint32_t dstSubRegOff = 0, srcSubRegOff = 0;
+  bool dstHasFixedSubregOffset = false;
+  if (dst->isNullReg())
+    dstHasFixedSubregOffset = true;
+  else
+    dstHasFixedSubregOffset =
+        dst->hasFixedSubregOffset(inst->getBuilder(), dstSubRegOff);
+
+  bool srcHasFixedSubregOffset = false;
+  if (isNullReg())
+    srcHasFixedSubregOffset = true;
+  else
+    srcHasFixedSubregOffset =
+        hasFixedSubregOffset(inst->getBuilder(), srcSubRegOff);
+
+  if (dstHasFixedSubregOffset && srcHasFixedSubregOffset) {
+    uint8_t dstStrideInBytes = (uint8_t)dst->getExecTypeSize();
+    uint16_t srcStride = 0;
+    getRegion()->isSingleStride(inst->getExecSize(), srcStride);
+    uint8_t srcStrideInBytes = (uint8_t)(srcStride * getTypeSize());
+    return checkFlatRegRegionFunc(dstStrideInBytes, (uint8_t)dstSubRegOff,
+                                    srcStrideInBytes,
+                                    (uint8_t)srcSubRegOff, exChannelWidth);
+  }
+  return false;
+}
+
 //
 // returns true if this SrcRegRegion has a fixed subreg offset (in bytes).
 // This is true only if

visa/G4_Operand.h

@@ -619,6 +619,11 @@ class G4_SrcRegRegion final : public G4_Operand {
   bool isDirectA0() const { return acc == Direct && base->isA0(); }
   bool isDirectAddress() const { return acc == Direct && base->isAddress(); }
   bool isScalar() const;
+  bool isFlatRegRegion(
+      uint8_t exChannelWidth,
+      std::function<bool(uint8_t dstStrideInBytes, uint8_t dstSubRegOffInBytes,
+                         uint8_t srcStrideInBytes, uint8_t srcSubRegOffInBytes,
+                         uint8_t exChannelWidth)> checkFlatRegRegionFunc);
 
   unsigned short ExRegNum(bool &) const;
   unsigned short ExSubRegNum(bool &);

visa/HWCaps.inc

@@ -873,4 +873,10 @@ bool alwaysAllowGlobalFlagOpt() const {
   // behavior for now.
   return getPlatform() <= GENX_TGLLP;
 }
+bool hasMulMacRSIssue() const {
+  if (getOption(vISA_hasMulMacRSIssue))
+    return true;
+
+  return VISA_WA_CHECK(getPWaTable(), Wa_18035690555);
+}
 // end HW capabilities

visa/Optimizer.h

@@ -239,6 +239,7 @@ class Optimizer {
   void linePlaneWA(G4_INST *inst);
   void fixSendSrcRegion(G4_INST *inst);
   void clearARFDependencies();
+  void mulMacRSWA();
   void clearSendDependencies();
   void loadThreadPayload();
   void addFFIDProlog();
@@ -284,6 +285,8 @@ class Optimizer {
   // In both case, the dst of inst_with_ip is used to save IP.
   void replaceIPWithCall(InstListType &insts, G4_INST *inst_with_ip);
 
+  void insertDummyAdd(G4_BB *bb, INST_LIST_ITER inst_it, int imm = 0);
+
   void insertDummyMad(G4_BB *bb, INST_LIST_ITER inst_it);
 
   void insertDummyCsel(G4_BB *bb, INST_LIST_ITER inst_it, bool newBB);

visa/SWWA.cpp

@@ -53,6 +53,19 @@ void Optimizer::insertDummyCompactInst() {
   bb->push_back(movInst);
 }
 
+// add (1|M0)   null<1>:uw   null<0;1,0>:uw       0x0:uw
+void Optimizer::insertDummyAdd(G4_BB *bb, INST_LIST_ITER inst_it, int imm) {
+  // Dst
+  auto nullDst = builder.createNullDst(Type_UW);
+  auto nullSrc0 = builder.createNullSrc(Type_UW);
+  auto immSrc1 = builder.createImm(imm, Type_UW);
+
+  auto addInst = builder.createBinOp(G4_add, g4::SIMD1, nullDst, nullSrc0,
+                                     immSrc1, InstOpt_WriteEnable, false);
+
+  bb->insertBefore(inst_it, addInst);
+}
+
 // Float and DP share same GRF cache.
 // Integer and Math shader same GRF cache.
 void Optimizer::insertDummyMad(G4_BB *bb, INST_LIST_ITER inst_it) {
@@ -3504,6 +3517,135 @@ void Optimizer::clearARFDependencies() {
     }
   }
 }
+void Optimizer::mulMacRSWA() {
+  auto hasGRFOverlap = [=](G4_Operand *A, G4_Operand *B) {
+    if (A->isNullReg() || !A->isGreg())
+      return false;
+    if (B->isNullReg() || !B->isGreg())
+      return false;
+
+    unsigned LB1 =
+        A->getLinearizedStart() / fg.builder->numEltPerGRF<Type_UB>();
+    unsigned RB1 = A->getLinearizedEnd() / fg.builder->numEltPerGRF<Type_UB>();
+    unsigned LB2 =
+        B->getLinearizedStart() / fg.builder->numEltPerGRF<Type_UB>();
+    unsigned RB2 = B->getLinearizedEnd() / fg.builder->numEltPerGRF<Type_UB>();
+
+    return (RB2 >= LB1 && RB1 >= LB2);
+  };
+
+  auto isBothMulClass = [](G4_INST *inst1, G4_INST *inst2) {
+    return (inst1->opcode() == G4_mul || inst1->opcode() == G4_mac) &&
+           (inst2->opcode() == G4_mul || inst2->opcode() == G4_mac);
+  };
+
+  auto isBothMaclClass = [](G4_INST *inst1, G4_INST *inst2) {
+    // In vISA, only G4_mach will be used. IGA will change it G4_macl according
+    // to certain conditions.
+    return (inst1->opcode() == G4_mach) &&
+           (inst2->opcode() == G4_mach);
+  };
+
+  auto checkFlatRegRegionFunc =
+      [](uint8_t dstStrideInBytes, uint8_t dstSubRegOffInBytes,
+         uint8_t srcStrideInBytes, uint8_t srcSubRegOffInBytes,
+         uint8_t exChannelWidth) -> bool {
+    return ((dstSubRegOffInBytes == srcSubRegOffInBytes) &&
+            (dstStrideInBytes == srcStrideInBytes) &&
+            (dstStrideInBytes % exChannelWidth == 0));
+  };
+
+  for (auto bb : fg) {
+    G4_INST *prevInst = nullptr;
+    INST_LIST_ITER ii = bb->begin();
+
+    while (ii != bb->end()) {
+      G4_INST *inst = *ii;
+
+      if (inst->getNumSrc() != 2 || inst->tokenHonourInstruction() ||
+          inst->isIntrinsic()) {
+        prevInst = nullptr;
+        ii++;
+        continue;
+      }
+
+      if (!prevInst) {
+        prevInst = inst;
+        ii++;
+        continue;
+      }
+
+      uint8_t exChannelWidth = (uint8_t)TypeSize(inst->getExecType());
+
+      // Issue 1:
+      // MUL opcode class = {MUL, MAC}
+      // MACL opcode class = {MACL, MACH}
+      //
+      // Issue is present for MUL opcode class  OR MACL opcode class (both
+      // prev/current instruction should belong to the same opcode class)
+      // 1. prev instructions src1 has REGIONING/SCALAR
+      // 2. current instruction src1 is FLAT and shares the same src1 as prev
+      //
+      // instruction Issue is not present for below cases.
+      // 1. prev instruction is FLAT and current instruction has
+      // REGIONING/SCALAR
+      // 2. prev/current both are FLAT
+      // 3. prev/current both has REGIONING/SCALAR
+      // 4. One instruction is in MUL opcode class and the other instruction
+      // is in MACL opcode class
+      if (isBothMulClass(prevInst, inst) || isBothMaclClass(prevInst, inst)) {
+        G4_Operand *prevSrc1 = prevInst->getSrc(1);
+        G4_Operand *curSrc1 = inst->getSrc(1);
+
+        if (prevSrc1->isGreg() &&
+            prevSrc1->isSrcRegRegion() && curSrc1 != nullptr &&
+            curSrc1->isGreg() && curSrc1->isSrcRegRegion()) { // All regions
+
+          if (!prevSrc1->asSrcRegRegion()->isFlatRegRegion(
+                  exChannelWidth, checkFlatRegRegionFunc) &&
+              curSrc1->asSrcRegRegion()->isFlatRegRegion(
+                  exChannelWidth, checkFlatRegRegionFunc) &&
+              hasGRFOverlap(
+                  prevSrc1,
+                  curSrc1)) { // none flat vs flat regions, and overlap
+            // WorkAround: Insert dummy instruction that can break src1 RS
+            // chain between regioning MUL instruction and FLAT MULK
+            // instruction (IMMEDIATE operand can be used  for src1 to break
+            // the RS chain)
+            insertDummyAdd(bb, ii);
+          }
+        }
+      }
+
+      // Issue 2
+      // prev.instruction is non-MUL opcode class instruction AND non-MACL
+      // opcode class instruction has(FLAT or Regioning / Scalar) src1 and
+      // current Instruction is MACL opcode class
+      // instruction AND has FLAT regioning AND shares the same src1 has the
+      // prev.instruction,
+      if (inst->opcode() == G4_mach) {
+        G4_Operand *prevSrc1 = prevInst->getSrc(1);
+        G4_Operand *curSrc1 = inst->getSrc(1);
+
+        if (prevSrc1->isGreg() &&
+            prevSrc1->isSrcRegRegion() && curSrc1 != nullptr &&
+            curSrc1->isGreg() && curSrc1->isSrcRegRegion()) {
+          if (prevInst->opcode() != G4_mach && prevInst->opcode() != G4_mul &&
+              prevInst->opcode() != G4_mac) {
+            if (curSrc1->asSrcRegRegion()->isFlatRegRegion(
+                    exChannelWidth, checkFlatRegRegionFunc) &&
+                hasGRFOverlap(prevSrc1, curSrc1)) {
+              insertDummyAdd(bb, ii, 1);
+            }
+          }
+        }
+      }
+
+      prevInst = inst;
+      ii++;
+    }
+  }
+}
 
 // change the send src0 region to be consistent with assembler expectation
 // We do it here instead of HW conformity since they only affect binary encoding
@@ -3778,6 +3920,10 @@ void Optimizer::HWWorkaround() {
     clearSendDependencies();
   }
 
+  if (builder.hasMulMacRSIssue()) {
+    mulMacRSWA();
+  }
+
   if (builder.needResetA0forVxHA0()) {
     // reset a0 to 0 at the beginning of a shader.
     // The goal of this initialization is to make sure that there is no

visa/include/VISAOptionsDefs.h

@@ -757,3 +757,4 @@ DEF_VISA_OPTION(vISA_EnableProgrammableOffsetsMessageBitInHeader, ET_BOOL,
                 NULLSTR, UNUSED, false)
 DEF_VISA_OPTION(vISA_staticProfiling, ET_BOOL, "-staticProfiling", UNUSED, true)
 DEF_VISA_OPTION(vISA_staticBBProfiling, ET_BOOL, "-staticBBProfiling", UNUSED, false)
+DEF_VISA_OPTION(vISA_hasMulMacRSIssue, ET_BOOL, "-hasMulMacRSIssue", UNUSED, false)