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

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

jmmartinez accepted this revision.
jmmartinez added a comment.
This revision is now accepted and ready to land.

Just a few minor suggestions. Otherwise it looks good to me.



================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUCodeGenPrepare.cpp:1497
+  for (const Value *U : I.users()) {
+    if (isa<PHINode>(U) && !canBreakPHINode(*cast<PHINode>(U)))
+      return BreakPhiNodesCache[&I] = false;
----------------
I'd use a dyn_cast instead of the isa + cast. Even if it makes the loop a bit bigger.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUCodeGenPrepare.cpp:1498
+    if (isa<PHINode>(U) && !canBreakPHINode(*cast<PHINode>(U)))
+      return BreakPhiNodesCache[&I] = false;
+  }
----------------
We can remove the assignment for the false case, since it's already set to false, right?


================
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();
+}
----------------
Pierre-vh wrote:
> jmmartinez wrote:
> > 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);
> > ```
> I think it's a bit more readable if I keep the current style, in the loop I still need to extract VecSrc anyway so IMO it's clearer to do:
> 
> ```
>     const auto *VecSrc = IE->getOperand(0);
>     if (isa<Instruction>(VecSrc) && !areInSameBB(VecSrc, IE))
>       return false;
> ```
>     const auto *VecSrc = IE->getOperand(0);
>     if(isOperandInSameBB(IE, 0))` // it's identical to VecSrc but looks different for no particular reason
>       return false;
> ```
> 
You're right, it looks weird.

What about using an `const Use &` then ? `Use&` converts implicitely to `Value*`, but it also keeps track of the `User` instruction.

```
const auto *VecSrc = IE->getOperand(0);
if (isa<Instruction>(VecSrc) && !areInSameBB(VecSrc, IE))
  return false;
```
would become
```
const Use& VecSrc = IE->getOperandUse(0);
if(isOperandInSameBBAsUser(VecSrc))
  return false;
```

With

```
static bool isOperandInSameBBAsUser(const Use& Op) {
  Instruction *OpI = dyn_cast<Instruction>(&Op);
  retrun !OpI || OpI->getParent() == cast<Instruction>(Op.getUser())->getParent();
}
```
---
I was also wondering. What if the operand is an `Argument`? (I guess there are no relevant cases to really care about it)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150266