[flang-commits] [PATCH] D127805: Bitwise comparison intrinsics

[Jean Perier via Phabricator via flang-commits]

jeanPerier added a comment.

Thanks for the update !  A few more nits in the constant expression folding code.



================
Comment at: flang/lib/Evaluate/fold-logical.cpp:23
+static std::optional<Expr<SomeType>> zeroExtend(Constant<T> *c) {
+  auto r = value::Integer<128>::ConvertUnsigned(*c->GetScalarValue());
+  return AsGenericExpr(Constant<LargestInt>{std::move(r.value)});
----------------
`Scalar<LargestInt>::ConvertUnsigned` should  also work here and would be more generic.


================
Comment at: flang/lib/Evaluate/fold-logical.cpp:24
+  auto r = value::Integer<128>::ConvertUnsigned(*c->GetScalarValue());
+  return AsGenericExpr(Constant<LargestInt>{std::move(r.value)});
+}
----------------
The bitwise intrinsic operands may be array constant, and this case must also be foldable in constant expression.
In that case, I think you could use c->values() to go through all the element values and convert them, and to create a new `Constant<LargestInt>{std::move(zextValues), std::move(c->shape())}` passing the previous shape.


================
Comment at: flang/lib/Evaluate/fold-logical.cpp:86
+        constArgs[i] = AsGenericExpr(Constant<LargestInt>{std::move(*x)});
+      } else if (auto *c{UnwrapConstantValue<Int1>(args[i])}) {
+        constArgs[i] = zeroExtend(c);
----------------
The flang code tends to prefer usage of std::visit when possible. The semantics code also tends to favor const& over passing via raw pointers. Here, I think you could do:

```
if (BOZLiteralConstant *...) {
 ...
} else if (auto *expr{UnwrapExpr<Expr<SomeInteger>>(args[i])}) {
  std::visit([&](const auto& typedIntExpr){
     using IntT = typename std::decay_t<decltype(x)>::Result;
     if (auto *c{UnwrapConstantValue<IntT >(typedIntExpr)}) {
        constArgs[i] = zeroExtend(*c);
     }
   }, expr->u) {
  }
}
```

The big advantage is that you do not have to assume too much about the list of possible integer types here.



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

https://reviews.llvm.org/D127805