Page MenuHomePhabricator

Invariants (and Assume Aligned) - Clang
ClosedPublic

Authored by hfinkel on Nov 30 2012, 3:55 PM.

Details

Summary

Clang patch for __builtin_assume_aligned; making use of LLVM IR invariants.

Diff Detail

Event Timeline

Unknown Object (User) added inline comments.Dec 1 2012, 3:32 AM
lib/CodeGen/CGBuiltin.cpp
400

You can replace this with a simple array.

gribozavr added inline comments.Dec 1 2012, 4:21 AM
lib/Sema/SemaChecking.cpp
2112–2113

Maybe three slashes to make it a Doxygen comment?

test/Sema/builtin-assume-aligned.c
28

Please add tests for every Diag() in semantic analysis: too many/too few args, dependent argument, a 64-bit alignment, a non-pow2 alignment, a non-ICE 3rd argument and a 3rd argument with bad type.

hfinkel updated this revision to Unknown Object (????).Dec 5 2012, 12:39 PM

Test cases have been enhanced. We now use @llvm.invariant.

Please update docs/LanguageExtensions.html with descriptions of these two new builtins.

lib/Sema/SemaChecking.cpp
2126

Should we just reject these cases? Seems like we would silently accept it without checking.

Or would it get checked during template instantiation?

2131–2134

What is the purpose of the '+' in '+llvm::Value::MaximumAlignment'?

test/Sema/builtin-assume-aligned.c
38

Still not enough :)

Please add:

__builtin_assume_aligned(a, 1ULL << 63);
int test9(int *a, int z) {
  a = __builtin_assume_aligned(a, z + 1); // not a constant expression
  return a[0];
}
template<int z>
int test10(int *a) {
  a = __builtin_assume_aligned(a, z + 1); // value-dependent
  return a[0];
}
void test10_inst(int *a) {
  test10<32>(a); // power of 2
  test10<42>(a); // not a power of 2
}
template<typename T>
int test11(int *a, T z) {
  a = __builtin_assume_aligned(a, z + 1); // value-dependent
  return a[0];
}
void test11_inst(int *a) {
  test11(a, 42);
}
int test12(int *a) {
  a = __builtin_assume_aligned(a, 32, "abc"); // wrong type
  return a[0];
}
int test13(int *a, int z) {
  a = __builtin_assume_aligned(a, z); // not a constant
  return a[0];
}
hfinkel updated this revision to Unknown Object (????).Dec 5 2012, 3:47 PM

More, and more explicit, test cases. Removed unneeded '+' operators. Added documentation.

I don't have more comments, so this looks good to me, but please wait for another review.

docs/LanguageExtensions.html
1400 ↗(On Diff #441)

typo: he -> the

1409–1412 ↗(On Diff #441)

Same comment as for LLVM intrinsic:

What about adding explicitly: "If the condition does not hold during execution, the behavior is undefined"?

  • Original Message -----

From: "Dmitri Gribenko" <gribozavr@gmail.com>
To: hfinkel@anl.gov
Cc: gribozavr@gmail.com, cfe-commits@cs.uiuc.edu, chandlerc@gmail.com
Sent: Wednesday, December 5, 2012 5:57:58 PM
Subject: Re: [PATCH] Invariants (and Assume Aligned) - Clang

I don't have more comments, so this looks good to me, but please
wait for another review.

Thanks! I will do (this also depends on the LLVM patch(es); and those would need to go in first).

-Hal

================
Comment at: docs/LanguageExtensions.html:1409-1412
@@ +1408,6 @@
+
+<p>The boolean argument to this function is defined to be true. The
optimizer
+may analyze the expression used to compute the argument and deduce
from that
+analysis information used to optimize the program.
+</p>
+


Same comment as for LLVM intrinsic:

What about adding explicitly: "If the condition does not hold during
execution, the behavior is undefined"?

================
Comment at: docs/LanguageExtensions.html:1400
@@ +1399,3 @@
+
+ if (x == 0) // he optimizer may short-circuit this check using the
invariant.
+ return do_something();


typo: he -> the

http://llvm-reviews.chandlerc.com/D149

  • Original Message -----

From: "Dmitri Gribenko" <gribozavr@gmail.com>
To: hfinkel@anl.gov
Cc: gribozavr@gmail.com, cfe-commits@cs.uiuc.edu, chandlerc@gmail.com
Sent: Wednesday, December 5, 2012 5:57:58 PM
Subject: Re: [PATCH] Invariants (and Assume Aligned) - Clang

I don't have more comments, so this looks good to me, but please
wait for another review.

Thanks! I will do (this also depends on the LLVM patch(es); and those would need to go in first).

-Hal

================
Comment at: docs/LanguageExtensions.html:1409-1412
@@ +1408,6 @@
+
+<p>The boolean argument to this function is defined to be true. The
optimizer
+may analyze the expression used to compute the argument and deduce
from that
+analysis information used to optimize the program.
+</p>
+


Same comment as for LLVM intrinsic:

What about adding explicitly: "If the condition does not hold during
execution, the behavior is undefined"?

================
Comment at: docs/LanguageExtensions.html:1400
@@ +1399,3 @@
+
+ if (x == 0) // he optimizer may short-circuit this check using the
invariant.
+ return do_something();


typo: he -> the

http://llvm-reviews.chandlerc.com/D149

A couple of quality-of-implementation things that would be nice to implement:

  • a warning if __builtin_assume_aligned is used to weaken alignment
  • extend th undefined behavior sanitizer with codegen for these builtins so that alignment and other invariants are checked at runtime
hfinkel updated this revision to Diff 11337.Jul 12 2014, 8:46 AM
hfinkel added reviewers: gribozavr, rsmith.
hfinkel removed a subscriber: gribozavr.

Rebased (and also hooked up MSVC's assume along with builtin_assume).

An updated version of the associated LLVM patches is coming soon.

hfinkel updated this revision to Diff 11341.Jul 12 2014, 8:25 PM

Use +llvm::Value::MaximumAlignment in Sema to force evaluation and prevent errors like this (in Debug builds):

libclang.so: undefined reference to `llvm::Value::MaximumAlignment'

(which is the same thing that is done in lib/Transforms/Utils/Local.cpp)

rsmith added inline comments.Jul 16 2014, 4:14 PM
docs/LanguageExtensions.rst
1267–1269

You should indicate whether and when the condition is evaluated here. IIRC, MS' __assume does not evaluate its argument, so if this builtin differs from that we should be explicit about that difference.

lib/AST/ExprConstant.cpp
6095

Please also add handling of BI__builtin_assume to VoidExprEvaluator.

lib/CodeGen/CGBuiltin.cpp
407

Skip this if the OffsetValue is the ConstantInt 0? (I'm kinda surprised the IRBuilder doesn't already do that...)

418

Is this really correct? As noted above, I think __assume doesn't evaluate its argument. Perhaps I'm mistaken.

lib/Sema/SemaChecking.cpp
38

I'm not happy about this dependency. But see below...

2069

This is a bit confusing. These seem to be two entirely unrelated facts (the alignment is argument 1).

2078–2081

What's the purpose of this check? If I want to claim that my pointer is 0x8000'0000'0000ull bytes aligned, I don't see why we should diagnose that, even if LLVM can't represent that alignment. CodeGen doesn't care about this being a valid LLVM alignment.

2084

This seems like an odd diagnostic ID to reuse. If you want to use it, please change its name to not mention an attribute.

2092–2093

I don't see how LLVM's supported alignment values have any impact here?

2096

Again, this is a weird diagnostic ID to reuse. This has nothing to do with converted constant expressions. (And you never even tried to do any conversion, so you can't reasonably claim the argument is not implicitly convertible to a particular type.)

Instead of all this, please perform the appropriate conversion from the argument to type size_t:

InitializedEntity Entity = InitializedEntity::InitializeParameter(Context, Context.getSizeType(), false);
Arg = PerformCopyInitialization(Entity, SourceLocation(), Arg);
if (Arg.isInvalid()) return true;

I'll update, thanks!

docs/LanguageExtensions.rst
1267–1269

Interesting point. MS's documentation says, "no code is generated", but does not otherwise specify the semantics of what is done with the arguments. With the LLVM intrinsic, although IR is generated, no assembly is generated.

Are you asking whether assume is more like decltype? Or could assume(*a > 0) trap if a were nullptr? I'm pretty sure that the answers are no and yes, respectively. MS's documentation specifically talks about using __assume as a production-build replacement for assert, and I assume so must evaluate its inputs in a similar-enough way to make that workable.

hfinkel added inline comments.Jul 16 2014, 4:52 PM
docs/LanguageExtensions.rst
1267–1269

After writing that, it occurred to me that whether or not assert evaluates its arguments depends on the preprocessor state, so there is little to infer from that comparison.

What is true is that MS has a compiler warning: C4557 (The value passed to an assume statement was modified.) which triggers on code like assume(i++).

hfinkel added inline comments.Jul 16 2014, 5:04 PM
docs/LanguageExtensions.rst
1267–1269

echristo just confirmed for me on IRC that __assume does not evaluate its arguments. I'll need to filter for side effects.

Thanks! Will post updated patch soon.

docs/LanguageExtensions.rst
1267–1269

Done.

lib/AST/ExprConstant.cpp
6095

Done.

lib/CodeGen/CGBuiltin.cpp
407

Done.

418

No, you're right. I added a side-effects check.

lib/Sema/SemaChecking.cpp
38

Removed.

2069

Agreed. I removed the bit about argument 0 (I'm not sure it really adds anything).

2078–2081

Good point.

2084

Renamed.

2092–2093

Fair enough.

2096

Thanks!

hfinkel updated this revision to Diff 11587.Jul 17 2014, 10:14 AM

Rebased and updated per Richard's comments.

hfinkel updated this revision to Diff 11702.Jul 20 2014, 12:02 PM

The LLVM intrinsic has been renamed to llvm.assume (from llvm.invariant) based on community feedback.

hfinkel updated this revision to Diff 11703.Jul 20 2014, 2:45 PM

Same functionality, but extracted a EmitAlignmentAssumption CGF helper function to make it easier for other code to also emit alignment assumptions.

hfinkel updated this revision to Diff 13227.Sep 3 2014, 2:57 PM

Moved .cpp test from Sema to SemaCXX (and rebased).

hfinkel accepted this revision.Sep 7 2014, 4:08 PM
hfinkel added a reviewer: hfinkel.

(will close)

This revision is now accepted and ready to land.Sep 7 2014, 4:08 PM
hfinkel closed this revision.Sep 7 2014, 4:08 PM

r217349, thanks!