[PATCH] D125627: [SCEV] Serialize function calls in function arguments.

[NAKAMURA Takumi via Phabricator via llvm-commits]

chapuni added inline comments.


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:1807
+      const auto *LHSz = getZeroExtendExpr(LHS, Ty, Depth + 1);
+      const auto *RHSz = getZeroExtendExpr(RHS, Ty, Depth + 1);
+      return getURemExpr(LHSz, RHSz);
----------------
nikic wrote:
> I'd just reassign LHS and RHS here (and below). The LHSz/RHSz names are bit unusual.
I was afraid since LHS and RHS are captured and assigned by matchURem(const SCEV*, const SCEV*&, const SCEV*&)
I thought they might be dedicated to matchURem().

How about;
- Rename original LHS,RHS to LHSURem,RHSURem
- Use LHS,RHS as generic instead of my LHSz,RHSz


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:7576
+    if (isKnownNonNegative(LHS = getSCEV(U->getOperand(0))) &&
+        isKnownNonNegative(RHS = getSCEV(U->getOperand(1))))
+      return getUDivExpr(LHS, RHS);
----------------
nikic wrote:
> Please move these as separate assignments before the `if` (here and below).
I avoided to evaluate RHS if the former condition (LHS) was false.
Could I split conditions? Or evaluate both LHS and RHS before conditions?

```
LHS =  = getSCEV(U->getOperand(0);
if (isKnownNonNegative(LHS))) {
  RHS = getSCEV(U->getOperand(1);
  if (isKnownNonNegative(RHS)))
    return getUDivExpr(LHS, RHS);
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125627