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

[Pierre van Houtryve via Phabricator via llvm-commits]

Pierre-vh 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();
+}
----------------
jmmartinez wrote:
> 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)
I'm still not convinced it needs changing :) 
I think the current version is easy enough to understand - it's a simple function with a simple name, and I think there's value in that

As for `Argument`s, they're intentionally not supported because they'll be lowered to a `CopyFromReg` anyway (I think), so we have no opportunity to simplify the ExtractElement instructions we'd insert.

I also think vector args are somewhat uncommon, I rarely encounter vector values in parameters, often people just pass an array of data coming from the host or something like that and you get `load <N x T> %arg` instead, so they're not worth considering unless a good counter-example is found, IMO.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150266