[PATCH] D150266: [AMDGPU] Improve PHI-breaking heuristics in CGP

[Juan Manuel Martinez Caamaño via Phabricator via llvm-commits]

jmmartinez added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUCodeGenPrepare.cpp:1405-1409
+static bool areInSameBB(const Value *A, const Value *B) {
+  const auto *IA = dyn_cast<Instruction>(A);
+  const auto *IB = dyn_cast<Instruction>(B);
+  return IA && IB && IA->getParent() == IB->getParent();
+}
----------------
Nit (very optional): I think that if we made `areInSameBB` into `isOperandInSameBB` (see below) the code becomes slightly simpler

```
static bool isOperandInSameBB(const Instruction& I, unsigned Op) {
  if(Instruction *OpI = dyn_cast<Instruction>(I->getOperand(Op))) {
    return Op->getParent() == I.getParent();
  }
  return true;
}
```

---

```
    if (isa<Instruction>(VecSrc) && !areInSameBB(VecSrc, IE))
      return false;
```
becomes
```
    if (!areInSameBB(IE, 0))
      return false;
```

and

```
    return isa<Constant>(SV->getOperand(1)) ||
           areInSameBB(SV, SV->getOperand(0)) ||
           areInSameBB(SV, SV->getOperand(1));
```
becomes
```
    return areInSameBB(SV, 0) || areInSameBB(SV, 1);
```


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUCodeGenPrepare.cpp:1469-1475
+  if (auto It = BreakPhiNodesCache.find(&I); It != BreakPhiNodesCache.end())
+    return It->second;
+
+  // This function may recurse, so to guard against infinite looping, this PHI
+  // is conservatively considered unbreakable until we reach a conclusion.
+  bool &Result = BreakPhiNodesCache[&I];
+  Result = false;
----------------
You can use the result of `DenseMap::insert` to do a single lookup instead of two (one for checking if the element is in the map and another to insert the false value).


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUCodeGenPrepare.cpp:1474
+  // is conservatively considered unbreakable until we reach a conclusion.
+  bool &Result = BreakPhiNodesCache[&I];
+  Result = false;
----------------
Are references to DenseMap elements guaranteed to be valid after insertion?

If not, the recursive call to `canBreakPhiNode` may insert new values into the cache and invalidate the references held by the parent calls. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150266/new/

https://reviews.llvm.org/D150266