[PATCH] D141568: [SCEV] Support SMin/Umin for GetMinTrailingZeros

Joshua Cao via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Jan 12 08:32:38 PST 2023


caojoshua added inline comments.


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:6342
+  if (isa<SCEVAddExpr>(S) || isa<SCEVAddRecExpr>(S) || isa<SCEVSMaxExpr>(S) ||
+      isa<SCEVUMaxExpr>(S) || isa<SCEVSMinExpr>(S) || isa<SCEVUMinExpr>(S)) {
     // The result is the min of all operands results.
----------------
nikic wrote:
> While here, would you mind supporting UMinSeq as well?
Yeah sure. Let me test this later. I think we can simplify this to

```
isa<SCEVMinMaxExpr> || isa<SCEVSequentialMinMaxExpr>
```


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:6346
     uint32_t MinOpRes = GetMinTrailingZeros(M->getOperand(0));
-    for (unsigned i = 1, e = M->getNumOperands(); MinOpRes && i != e; ++i)
-      MinOpRes = std::min(MinOpRes, GetMinTrailingZeros(M->getOperand(i)));
+    for (unsigned I = 1; MinOpRes && I != M->getNumOperands(); ++I)
+      MinOpRes = std::min(MinOpRes, GetMinTrailingZeros(M->getOperand(I)));
----------------
nikic wrote:
> Any reason to deviate from the previous structure?
`i` -> `I` because [LLVM coding standards state](https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly) that `Variable names should be nouns (as they represent state). The name should be camel case, and start with an upper case letter (e.g. Leader or Boats)`. And cause my editor is smart enough to know we are using LLVM coding standards and recommend this change.

No need to assign assign `getNumOperands()`  to `E`. Can just compare directly against `getNumOperands()`, and remove unnecessary `E` variable. M is `const`, so we know the number of operands will not change through iterations barring some unsafe casting (I think this is how C++ const works).

No strong opinions from me. This deviates from code style in rest of function. I can update the rest of the function as a NFC followup to make sure everything is consistent.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141568



More information about the llvm-commits mailing list