[PATCH] D102367: [LowerConstantIntrinsics] only accept ConstantData other than UndefValue as constant

[Richard Smith - zygoloid via Phabricator via llvm-commits]

rsmith added a comment.

In D102367#2755587 <https://reviews.llvm.org/D102367#2755587>, @nikic wrote:

> Can you explain why UndefValue needs to be excluded? It's not mentioned in the summary, and the test case does not cover it.

The intrinsic is only supposed to return `i1 true` for manifest constants -- for example, `@llvm.is.constant.i32(i32 %n)` should return `i1 true` only if its argument has been reduced to `i32 N` for some integer value `N`. While I think it would be correct to return `i1 true` when passing `undef` (because it's correct to replace `i32 undef` with, for example, `i32 0`), it would also be correct to return `i1 false`, and generally we want this intrinsic to conservatively return `false` whenever it has not reduced its argument to a manifest constant, so returning `false` on `undef` seemed like the better choice to me. And it certainly seems like the better choice for `@llvm.is.constant.iN` as an implementation of `__builtin_constant_p`, which I think was the intent. As a special case, it would seem quite broken if `__builtin_constant_p(e)` evaluates to `true` if `e` is a non-constant expression that can be reduced to `undef`!

On the other hand, for the "generalized" form that takes an arbitrary type, it seems much more reasonable for `undef` to be treated as constant, not least because that means we won't distinguish between "secret undef" hiding in unrepresented padding of a non-packed struct and explicit undef in a corresponding packed struct. I don't know why this intrinsic was generalized in this way, though -- it seems to be designed around the needs of `__builtin_constant_p`, which doesn't need or want support for anything other than integers -- so it's hard to see what the proper behavior would be. I wonder if perhaps we should remove support for anything other than `iN`.



================
Comment at: llvm/lib/Transforms/Scalar/LowerConstantIntrinsics.cpp:55-57
+    if (auto *CA = dyn_cast<ConstantAggregate>(V))
+      for (Value *Op : CA->operands())
+        WorkList.push_back(Op);
----------------
The `ConstantAggregate` case appears to be unreachable because a `ConstantAggregate` is not a `ConstantData`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102367