[LowerConstantIntrinsics] reuse isManifestLogic from ConstantFolding

[nickdesaulniers]

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

[rsmith]

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]

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

[nickdesaulniers]

In 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.

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.

[nickdesaulniers]

• add tests for undef, fix check all DFS

[nickdesaulniers]

In D102367#2755619, @nickdesaulniers wrote:

In 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.

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 }

[rsmith]

In 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.

llvm/lib/Transforms/Scalar/LowerConstantIntrinsics.cpp

55–57

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

[nickdesaulniers]

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.

[nickdesaulniers]

• fix EarlyCSE, InstCombine, ConstantAggregate

[nickdesaulniers]

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]

llvm/lib/Analysis/ConstantFolding.cpp

1830–1835

probably should just update isManifestConstant

[nickdesaulniers]

• 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.

[nickdesaulniers]

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]

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]

In D102367#2755705, @nickdesaulniers wrote:

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]

• hoist isManifestConstant to method of Constant, drop undef special case

[nickdesaulniers]

In D102367#2756322, @nikic wrote:

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.

[rsmith]

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.

[george.burgess.iv]

one drive-by nit and lgtm -- thanks!

llvm/lib/IR/Constants.cpp

829

nit: llvm style prefers no elses after returns

[nickdesaulniers]

• style nits

[kees]

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!

llvm/include/llvm/IR/Constant.h

public:
   static Constant *replaceUndefsWith(Constant *C, Constant *Replacement);
 
   /// Merges undefs of a Constant with another Constant, along with the
   /// undefs already present. Other doesn't have to be the same type as C, but
   /// both must either be scalars or vectors with the same element count. If no
   /// changes are made, the constant C is returned.
   static Constant *mergeUndefsWith(Constant *C, Constant *Other);
 
+  /// Return true if a constant is ConstantData or a ConstantAggregate or
+  /// ConstantExpr that contain only ConstantData.
+  bool isManifestConstant() const;
+
 private:
   enum PossibleRelocationsTy {
     /// This constant requires no relocations. That is, it holds simple
     /// constants (like integrals).
     NoRelocation = 0,
 
     /// This constant holds static relocations that can be resolved by the
     /// static linker.

llvm/lib/Analysis/ConstantFolding.cpp

double getValueAsDouble(ConstantFP *Op) {
   if (Ty->isDoubleTy())
     return Op->getValueAPF().convertToDouble();
 
   bool unused;
   APFloat APF = Op->getValueAPF();
   APF.convert(APFloat::IEEEdouble(), APFloat::rmNearestTiesToEven, &unused);
   return APF.convertToDouble();
 }
 
-static bool isManifestConstant(const Constant *c) {
-  if (isa<ConstantData>(c)) {
-    return true;
-  } else if (isa<ConstantAggregate>(c) || isa<ConstantExpr>(c)) {
-    for (const Value *subc : c->operand_values()) {
-      if (!isManifestConstant(cast<Constant>(subc)))
-        return false;
-    }
-    return true;
-  }
-  return false;
-}
-
 static bool getConstIntOrUndef(Value *Op, const APInt *&C) {
   if (auto *CI = dyn_cast<ConstantInt>(Op)) {
     C = &CI->getValue();
     return true;
   }
   if (isa<UndefValue>(Op)) {
     C = nullptr;
     return true;
   }
   return false;
 }
 
 static Constant *ConstantFoldScalarCall1(StringRef Name,
                                          Intrinsic::ID IntrinsicID,
                                          Type *Ty,
                                          ArrayRef<Constant *> Operands,
                                          const TargetLibraryInfo *TLI,
                                          const CallBase *Call) {
   assert(Operands.size() == 1 && "Wrong number of operands.");
 
   if (IntrinsicID == Intrinsic::is_constant) {
     // We know we have a "Constant" argument. But we want to only
     // return true for manifest constants, not those that depend on
     // constants with unknowable values, e.g. GlobalValue or BlockAddress.
-    if (isManifestConstant(Operands[0]))
+    if (Operands[0]->isManifestConstant())
       return ConstantInt::getTrue(Ty->getContext());
     return nullptr;
   }
   if (isa<UndefValue>(Operands[0])) {
     // cosine(arg) is between -1 and 1. cosine(invalid arg) is NaN.
     // ctpop() is between 0 and bitwidth, pick 0 for undef.
     // fptoui.sat and fptosi.sat can always fold to zero (for a zero input).
     if (IntrinsicID == Intrinsic::cos ||
         IntrinsicID == Intrinsic::ctpop ||
         IntrinsicID == Intrinsic::fptoui_sat ||

llvm/lib/IR/Constants.cpp

if (!match(NewC[I], m_Undef()) && match(OtherEltC, m_Undef())) {
       FoundExtraUndef = true;
     }
   }
   if (FoundExtraUndef)
     return ConstantVector::get(NewC);
   return C;
 }
 
+bool Constant::isManifestConstant() const {
+  if (isa<ConstantData>(this))
+    return true;
+  if (isa<ConstantAggregate>(this) || isa<ConstantExpr>(this)) {
+    for (const Value *Op : operand_values())
+      if (!cast<Constant>(Op)->isManifestConstant())
+        return false;
+    return true;
+  }
+  return false;
+}
+
 //===----------------------------------------------------------------------===//
 //                                ConstantInt
 //===----------------------------------------------------------------------===//
 
 ConstantInt::ConstantInt(IntegerType *Ty, const APInt &V)
     : ConstantData(Ty, ConstantIntVal), Val(V) {
   assert(V.getBitWidth() == Ty->getBitWidth() && "Invalid constant for type");
 }

llvm/lib/Transforms/Scalar/LowerConstantIntrinsics.cpp

 #define DEBUG_TYPE "lower-is-constant-intrinsic"
 
 STATISTIC(IsConstantIntrinsicsHandled,
           "Number of 'is.constant' intrinsic calls handled");
 STATISTIC(ObjectSizeIntrinsicsHandled,
           "Number of 'objectsize' intrinsic calls handled");
 
 static Value *lowerIsConstantIntrinsic(IntrinsicInst *II) {
-  Value *Op = II->getOperand(0);
-
-  return isa<Constant>(Op) ? ConstantInt::getTrue(II->getType())
-                           : ConstantInt::getFalse(II->getType());
+  if (auto *C = dyn_cast<Constant>(II->getOperand(0)))
+    if (C->isManifestConstant())
+      return ConstantInt::getTrue(II->getType());
+  return ConstantInt::getFalse(II->getType());
 }
 
 static bool replaceConditionalBranchesOnConstant(Instruction *II,
                                                  Value *NewValue,
                                                  DomTreeUpdater *DTU) {
   bool HasDeadBlocks = false;
   SmallSetVector<Instruction *, 8> Worklist;
   replaceAndRecursivelySimplify(II, NewValue, nullptr, nullptr, nullptr,
                                 &Worklist);
   for (auto I : Worklist) {
     BranchInst *BI = dyn_cast<BranchInst>(I);
     if (!BI)
       continue;
     if (BI->isUnconditional())

llvm/test/Transforms/LowerConstantIntrinsics/constant-intrinsics.ll

; CHECK-NOT: llvm.is.constant
   %res2 = add i1 %x1, %x2
   %res3 = add i1 %res2, %x3
   %res4 = add i1 %res3, %x4
   %res5 = add i1 %res4, %x5
   %res6 = add i1 %res5, %x6
 
   ret i1 %res6
 }
+
+@real_mode_blob_end = external dso_local global [0 x i8], align 1
+define i1 @global_array() {
+; CHECK-LABEL: @global_array(
+; CHECK-NEXT: ret i1 false
+  %1 = call i1 @llvm.is.constant.i64(i64 ptrtoint ([0 x i8]* @real_mode_blob_end to i64))
+  ret i1 %1
+}