[PATCH] D52854: Use is.constant intrinsic for __builtin_constant_p

[Bill Wendling via Phabricator via cfe-commits]

void marked 9 inline comments as done.
void added a comment.

In https://reviews.llvm.org/D52854#1255755, @rsmith wrote:

> Essentially, you would add a `ConstExpr` node (or similar), teach `IgnoreImplicit` and friends to step over it


I've been trying to do this to no avail. Here's the code I was working with:

  //===----------------------------------------------------------------------===//
  // Wrapper Expressions.
  //===----------------------------------------------------------------------===//
  
  // ConstExpr is a wrapper class indicating that the expression must be constant.
  // This is useful for something like __builtin_constant_p(), which in some
  // contexts is required to be constant before generating LLVM IR.
  template <typename T>
  class ConstExpr : public Expr {
    T *E;
  
   public:
    ConstExpr(Expr *E) : E(E) {}
  
    T *getSubExpr() { return E; }
    const T *getSubExpr() const { return E; }
  
    static bool classof(const Stmt *T) {
      return T->getStmtClass() == ConstExprClass;
    }
  };

I did it this way because otherwise I would have to forward all calls to individual expression, which is ugly and made me want to cry. The issue is getting the `ConstExprClass` variable. Those variables are automatically generated via macro magick, but those macros aren't set up to handle templates.

Do I need to mangle the macros to support templates, or is there another way to achieve this?



================
Comment at: lib/AST/ExprConstant.cpp:8162
+      return Success(true, E);
+    if (isa<DeclRefExpr>(Arg->IgnoreParenCasts()) &&
+        E->getCanDelayEvaluation())
----------------
rsmith wrote:
> rsmith wrote:
> > Your `canDelayEvaluation` check does not appear to cover several of the cases we'd need to check for here. Eg:
> > 
> > ```
> > void f(int n) {
> >   enum E { a = __builtin_constant_p(n) }; // ok, a == 0, not an error because a's value is non-constant
> > ```
> > 
> > 
> Hmm, OK. Per https://bugs.llvm.org/show_bug.cgi?id=4898#c38, the correct check would be whether the reason we found the expression to be non-constant was that it tried to read the value of a local variable or function parameter. Eg, for:
> 
> ```
> void f(int x, int y) {
>   bool k = __builtin_constant_p(3 + x < 5 * y);
> ```
> 
> ... we should defer the `__builtin_constant_p` until after optimization. (And we should never do this if the expression has side-effects.) But given that your goal is merely to improve the status quo, not to exactly match GCC, this may be sufficient.
> 
> You should at least check that the variable is non-volatile, though. (More generally, check that `Arg` doesn't have side-effects.)
I had the side effect check in there before. I removed it for some reason. I'll re-add it.


================
Comment at: lib/Sema/SemaExpr.cpp:5694
 
+  MarkBuiltinConstantPCannotDelayEvaluation(InitExpr);
   return BuildCompoundLiteralExpr(LParenLoc, TInfo, RParenLoc, InitExpr);
----------------
rsmith wrote:
> Why are you marking this? This isn't (necessarily) a context where a constant expression is required. (In C, the overall initializer for a global would be, but a local-scope compound literal would not.)
> 
> Also, this should be in the `Build` function, not in the `ActOn` function.
The name may be misleading. It's looking to see if it should mark whether it can delay evaluation or not. I'll change its name to be clearer.


================
Comment at: lib/Sema/SemaExpr.cpp:5799
   E->setType(Context.VoidTy); // FIXME: just a place holder for now.
+  MarkBuiltinConstantPCannotDelayEvaluation(E);
   return E;
----------------
rsmith wrote:
> Likewise here, I don't see the justification for this.
I can't find an equivalent "Build..." for `InitList` expressions. Could you point me to it?


https://reviews.llvm.org/D52854