[PATCH] D144334: [Clang] Add C++2b attribute [[assume(expression)]]

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

So one thing to note here: I'm on the fence as to whether we want to implement this feature at all.  As was discussed extensively during the EWG meetings on this: multiple implementers are against this attribute for a variety of reasons, and at least 1 other implementer has stated they might 'implementer veto' this. I think there is discussion to be had among the code owners here as to whether we even want this.



================
Comment at: clang/include/clang/Basic/AttrDocs.td:2107
+  let Content = [{
+The ``assume`` attribute used to provide the optimizer with an expression that
+is defined to evaluate to ``true`` at the place the assumption occurs. The
----------------



================
Comment at: clang/include/clang/Basic/AttrDocs.td:2112
+violated during execution, the behavior is undefined. The argument itself is
+never evaluated, so any side effects of the expression will be discarded.
+
----------------
This last sentence is inaccurate.  As discussed in EWG, this attribute has some pretty painful side-effects (instantiation, lambda creation/etc).


================
Comment at: clang/test/CodeGenCXX/cxx2b-assume.cpp:2
+// RUN: %clang_cc1 -std=c++2b %s -emit-llvm -o - -triple x86_64-linux-gnu -disable-llvm-passes -DUSE_ASSUME | FileCheck %s
+// RUN: %clang_cc1 -std=c++2b %s -emit-llvm -o - -triple x86_64-linux-gnu -O3 | FileCheck --check-prefixes=CHECK-OPT-NOASSUME %s
+// RUN: %clang_cc1 -std=c++2b %s -emit-llvm -o - -triple x86_64-linux-gnu -O3 -DUSE_ASSUME | FileCheck --check-prefixes=CHECK-OPT %s
----------------
Having a CFE test depend on the optimizer like this is EXTREMELY frowned upon.  We shouldn't have clang tests depend on optimizer performance.  


================
Comment at: clang/test/CodeGenCXX/cxx2b-assume.cpp:55
+// CHECK-OPT-NEXT:   tail call void @llvm.assume(i1 %[[CMP]])
+// CHECK-OPT-NEXT:   ret i32 43
+//
----------------
nikic wrote:
> Isn't the assume expression supposed to be unevaluated?
We need MUCH more complicated tests (including just any expression/function call/etc) to ensure that the side-effects aren't going to cause problems, particularly in -O0.  I am somewhat confident that the middle-end is going to be willing to do the work to figure out that something passed to 'assume' is passed out correctly, but I'm not convinced that this will prevent all side-effects from escaping.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D144334/new/

https://reviews.llvm.org/D144334