[PATCH] D33420: [LIR] Strengthen the check for recurrence variable in popcnt/CTLZ

[Evgeny Stupachenko via Phabricator via llvm-commits]

evstupac added inline comments.


================
Comment at: lib/Transforms/Scalar/LoopIdiomRecognize.cpp:1042
+static bool checkRecurrence(Value *&VarX, Instruction *&DefX,
+                            BasicBlock *LoopEntry, PHINode *&PhiX) {
+  PhiX = dyn_cast<PHINode>(VarX);
----------------
davide wrote:
> davide wrote:
> > evstupac wrote:
> > > I wonder if you need PhiX as a parameter. It looks like it is unused after the call.
> > > Also it looks like the function does not modify anything, so you don't need references.
> > Isn't it used in `recognizeAndInsertCTLZ` ?
> Yes, I removed the refs.
>Isn't it used in recognizeAndInsertCTLZ ?
Yes. Right.
I was confused by function name "check". Maybe we could calculate PhiX before the check (outside the function)?
Or do like:

```
// checks that VarX and DefX create a loop recurrence and return corresponding phi node or NULL otherwise.
static PHINode *checkRecurrence(Value *VarX, Instruction *DefX, BasicBlock *LoopEntry) {
  PhiX = dyn_cast<PHINode>(VarX);
  if (PhiX && PhiX->getParent() == LoopEntry &&
      (PhiX->getOperand(0) == DefX || PhiX->getOperand(1) == DefX))
    return PhiX;
  return nullptr;
}
```


================
Comment at: lib/Transforms/Scalar/LoopIdiomRecognize.cpp:1140
 
       PHINode *Phi = dyn_cast<PHINode>(Inst->getOperand(0));
       if (!Phi || Phi->getParent() != LoopEntry)
----------------
Can we reuse the new function here for Inst and Inst->getOperand(0)?


================
Comment at: lib/Transforms/Scalar/LoopIdiomRecognize.cpp:1255
 
     PHINode *Phi = dyn_cast<PHINode>(Inst->getOperand(0));
     if (!Phi || Phi->getParent() != LoopEntry)
----------------
And here.


https://reviews.llvm.org/D33420