This is an archive of the discontinued LLVM Phabricator instance.

[LowerConstantIntrinsics] reuse isManifestLogic from ConstantFolding
ClosedPublic

Authored by nickdesaulniers on May 12 2021, 1:56 PM.

Details

Summary

GlobalVariables are Constants, yet should not unconditionally be
considered true for __builtin_constant_p.

Via the LangRef
https://llvm.org/docs/LangRef.html#llvm-is-constant-intrinsic:

This intrinsic generates no code. If its argument is known to be a
manifest compile-time constant value, then the intrinsic will be
converted to a constant true value. Otherwise, it will be converted
to a constant false value.

In particular, note that if the argument is a constant expression
which refers to a global (the address of which _is_ a constant, but
not manifest during the compile), then the intrinsic evaluates to
false.

Move isManifestConstant from ConstantFolding to be a method of
Constant so that we can reuse the same logic in
LowerConstantIntrinsics.

pr/41459

Diff Detail

Event Timeline

nickdesaulniers requested review of this revision.May 12 2021, 1:56 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 12 2021, 1:56 PM
rsmith added inline comments.May 12 2021, 3:13 PM
llvm/lib/Transforms/Scalar/LowerConstantIntrinsics.cpp
53–54

This looks backwards to me: I think we should only return true if all values in the worklist are suitable constants, not if any of them is.

nikic added a subscriber: nikic.May 12 2021, 3:18 PM

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

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

Looks like we disagree with GCC on:

int foo(void) {
    int i;
    return __builtin_constant_p(i);
}

Oh, that doesn't produce undef...hmm. @rsmith suggested it in https://bugs.llvm.org/show_bug.cgi?id=41459#c5. I'm happy to add a test for it, but I'm not sure precisely why @rsmith made that recommendation.

  • add tests for undef, fix check all DFS
nickdesaulniers marked an inline comment as done.May 12 2021, 3:51 PM

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

Looks like we disagree with GCC on:

int foo(void) {
    int i;
    return __builtin_constant_p(i);
}

Oh, that doesn't produce undef...hmm. @rsmith suggested it in https://bugs.llvm.org/show_bug.cgi?id=41459#c5. I'm happy to add a test for it, but I'm not sure precisely why @rsmith made that recommendation.

Seems like it's early-cse that messes that up:

declare i1 @llvm.is.constant.i32(i32 %a) nounwind readnone
define dso_local i32 @foo() #0 {
entry:
  %0 = call i1 @llvm.is.constant.i32(i32 undef)
  %1 = zext i1 %0 to i32
  ret i32 %1
}

opt -passes=early-cse y.ll -S

; ModuleID = 'y.ll'
source_filename = "y.ll"

; Function Attrs: convergent nofree nosync nounwind readnone willreturn
declare i1 @llvm.is.constant.i32(i32) #0

define dso_local i32 @foo() {
entry:
  ret i32 1
}

attributes #0 = { convergent nofree nosync nounwind readnone willreturn }

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.

llvm/lib/Transforms/Scalar/LowerConstantIntrinsics.cpp
55–57

The ConstantAggregate case appears to be unreachable because a ConstantAggregate is not a ConstantData.

While I think it would be correct to return i1 true when passing undef ..., it would also be correct to return i1 false

I think it's better to return i1 false, otherwise we diverge from GCC on the test case I provided above:

int foo(void) {
    int i;
    return __builtin_constant_p(i);
}

(and disagree with ourselves when optimizations are enabled). https://godbolt.org/z/P8onTznMj I'm fixing early-cse and instcombine now.

  • fix EarlyCSE, InstCombine, ConstantAggregate
nickdesaulniers marked an inline comment as done.May 12 2021, 5:33 PM
nickdesaulniers retitled this revision from [LowerConstantIntrinsics] only accept ConstantData other than UndefValue as constant to [Intrinsics] is_constant intrinsic on undef evaluates to false.May 12 2021, 5:37 PM
nickdesaulniers edited the summary of this revision. (Show Details)
nickdesaulniers added reviewers: nikic, regehr.
llvm/lib/Transforms/Scalar/EarlyCSE.cpp
1247–1251 ↗(On Diff #344993)

ah, this can be dropped now; it looks like early-cse and instcombine reuse the same constant folding logic, so only that needs updating to fix this for either pass.

nickdesaulniers planned changes to this revision.May 12 2021, 5:45 PM
nickdesaulniers added inline comments.
llvm/lib/Analysis/ConstantFolding.cpp
1830–1835

probably should just update isManifestConstant

  • remove EarlyCSE check, since it reuses ConstantFolding machinery
llvm/lib/Analysis/ConstantFolding.cpp
1830–1835

Ah, no, then InstCombine would fail to fold i1 @llvm.is.constant.i32(i32 undef) into i1 0.

llvm/lib/Analysis/ConstantFolding.cpp
1830–1835

though, perhaps earlycse/instcombine will fold aggregates containing undef to true rather than false? Hmm...what is meant by a "manifest constant?"

joerg added a comment.May 12 2021, 7:59 PM

One of the mots important use cases of __builtin_constant_p is ensuring that inline assembler variants that require immediate arguments are DCE'ed. There are some other use cases where it is used for compile-time assertions of invariants, so we do want to be as correct as possible. All that said, I fully agree that is.constant(undef) should fold to false as it is consistent with the immarg parameter attribute and other places. Even stronger, I would argue that it applies even to is.constant(freeze undef).

nikic added a comment.May 13 2021, 1:05 AM

While I think it would be correct to return i1 true when passing undef ..., it would also be correct to return i1 false

I think it's better to return i1 false, otherwise we diverge from GCC on the test case I provided above:

int foo(void) {
    int i;
    return __builtin_constant_p(i);
}

You will not be able to get consistent semantics by special-casing undef. Yes, for your above example your new implementation will now return false, but if you do some trivial change like replacing i with i & 1 it will return true again, because we fold undef & 1 to zero. Undef can always be refined to a constant value.

(and disagree with ourselves when optimizations are enabled)

This is not a problem per se, LangRef explicitly allows this: "The result also intentionally depends on the result of optimization passes".

llvm/lib/Analysis/ConstantFolding.cpp
1808–1809

Maybe you want to export this helper (or add it to Constant) and use it in LowerConstantIntrinsics? Seems like these should be using the same logic.

nickdesaulniers marked an inline comment as done.
  • hoist isManifestConstant to method of Constant, drop undef special case

You will not be able to get consistent semantics by special-casing undef. Yes, for your above example your new implementation will now return false, but if you do some trivial change like replacing i with i & 1 it will return true again, because we fold undef & 1 to zero. Undef can always be refined to a constant value.

Hmm...that's a fair point. For pr/41459, I don't particularly care about the undef case, so I'll remove that for now, so that we can fix LowerConstantIntrinsics.

nickdesaulniers retitled this revision from [Intrinsics] is_constant intrinsic on undef evaluates to false to [LowerConstantIntrinsics] reuse isManifestLogic from ConstantFolding.May 13 2021, 10:47 AM
nickdesaulniers edited the summary of this revision. (Show Details)
nickdesaulniers edited reviewers, added: joerg; removed: regehr.May 13 2021, 10:53 AM
nickdesaulniers added a subscriber: regehr.
rsmith accepted this revision.May 13 2021, 11:09 AM

LGTM, thanks!

The undef problem seems pretty fundamental here, and I don't think there's an easy fix. Let's consider that separately from this patch.

This revision is now accepted and ready to land.May 13 2021, 11:09 AM

one drive-by nit and lgtm -- thanks!

llvm/lib/IR/Constants.cpp
829

nit: llvm style prefers no elses after returns

nickdesaulniers marked an inline comment as done.May 13 2021, 12:14 PM
kees added a comment.May 13 2021, 11:41 PM

FWIW, I've tested this on a full Linux kernel build and I can confirm it's building correctly; I'm able to start my incremental FORTIFY fixes now. :) Thanks!