This is an archive of the discontinued LLVM Phabricator instance.

Implement constexpr __builtin_*_overflow
ClosedPublic

Authored by erichkeane on Jun 11 2018, 11:23 AM.

Diff Detail

Event Timeline

erichkeane created this revision.Jun 11 2018, 11:23 AM
rjmccall added inline comments.Jun 11 2018, 11:47 AM
lib/AST/ExprConstant.cpp
8260

The comment should explain *why* growth isn't possible (it's because we extended to the max-width type earlier).

I don't think a casual reader is going to understand what you're saying about TruncOrSelf (that APSInt doesn't have such an operation, but that we can safely simulate it with extOrTrunc because we'll never do an extension).

erichkeane marked an inline comment as done.
efriedma added inline comments.
lib/Sema/SemaChecking.cpp
210 ↗(On Diff #150808)

Can you split this change into a separate patch? Testcase:

int a() {
  const int x = 3;
  static int z;
  constexpr int * y = &z;
  return []() { return __builtin_sub_overflow(x,x,y); }();
}
erichkeane added inline comments.Jun 11 2018, 12:37 PM
lib/Sema/SemaChecking.cpp
210 ↗(On Diff #150808)

Can you clarify what you mean? That above testcase (with added captures) seems to work currently. What difference in behavior should I be expecting?

efriedma added inline comments.Jun 11 2018, 1:54 PM
lib/Sema/SemaChecking.cpp
210 ↗(On Diff #150808)

The testcase should type-check as-is, without adding any captures. Reading the value of a constexpr variable, or a const variable of integer type, isn't an odr-use. This doesn't work correctly at the moment because the lvalue-to-rvalue conversions are missing from the AST. Compare to the following:

int a() {
  const int x = 3;
  static int z;
  constexpr int * y = &z;
  return []() { return __builtin_sub_overflow((int)x,(int)x,(int*)y); }();
}
erichkeane added inline comments.Jun 11 2018, 1:57 PM
lib/Sema/SemaChecking.cpp
210 ↗(On Diff #150808)

Ah, got it! I 'll split that off into a separate patch with those test cases now. Look for the review soon!

Alright, done here: https://reviews.llvm.org/D48053

This one'll require some rebasing on that change, but I'm not sure how to do it in SVN. Therefore, I'll just rebase this one when it comes to commit it.

-Erich

Thanks, comment change looks good. LGTM.

Separated out the other patch as Eli suggested (which has now been committed), and rebased this patch on top of it.

efriedma accepted this revision.Jun 13 2018, 11:42 AM

I'd like to see a couple of testcases ensuring the return value is correct on overflow (we had a problem with that for __builtin_mul_overflow in the past).

Otherwise LGTM.

This revision is now accepted and ready to land.Jun 13 2018, 11:42 AM

I believe my tests DO validate the return value correctly, don't they? It uses a sentinel, but the ternary should check that return value, right?

Or is there an obvious thing I'm missing?

Oh, sorry, I mixed up the two values. I meant that you should add a couple testcases to ensure the stored value is correct on overflow.

This revision was automatically updated to reflect the committed changes.