[PATCH] D100118: [clang] RFC Support new builtin __arithmetic_fence to control floating point optimization, and new clang option fprotect-parens

2021-04-26 Thread Melanie Blower via Phabricator via cfe-commits 
mibintc added a comment.

some inline comments for reviewers




Comment at: clang/include/clang/Basic/TargetInfo.h:1162
   /// the language based on the target options where applicable.
-  virtual void adjust(LangOptions );
+  virtual void adjust(DiagnosticsEngine , LangOptions );
 

There's no good place to diagnose when LangOptions and TargetOptions conflict, 
I see "conflict" diagnostics being emitted in clang/CodeGen when creating the 
func or module, which seems wrong to me. On the other hand, the "adjust" 
function makes a good place to do the reconciliation I think. This is the 
change that could be pulled out in a refactoring and committed prior to this 
patch. What do you think? 



Comment at: clang/include/clang/Driver/Options.td:4300
 defm pack_derived : BooleanFFlag<"pack-derived">, Group;
-defm protect_parens : BooleanFFlag<"protect-parens">, Group;
+//defm protect_parens : BooleanFFlag<"protect-parens">, Group;
 defm range_check : BooleanFFlag<"range-check">, Group;

@jansvoboda11 This is a gfortran option, but I don't think there's a way to 
allow the same option spelling for gfortran and clang. Other clang options, 
like -short-enum, also have been pulled out of gfortran group, and the gfortran 
option test is an expected-fail test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100118

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100118: [clang] RFC Support new builtin __arithmetic_fence to control floating point optimization, and new clang option fprotect-parens

2021-04-26 Thread Melanie Blower via Phabricator via cfe-commits 
mibintc updated this revision to Diff 340598.
mibintc added a reviewer: aaron.ballman.
mibintc added a comment.
Herald added subscribers: jansvoboda11, dexonsmith, lxfind, dang, kerbowa, 
kbarton, aheejin, jgravelle-google, sbc100, nhaehnle, jvesely, nemanjai, 
dschuff.

I think this patch is complete except it needs to wait until the parent patch 
is finished.  Also some re-factoring may be desireable, I'll add some inline 
comments about that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100118

Files:
  clang/docs/UsersManual.rst
  clang/include/clang/Basic/Builtins.def
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Basic/LangOptions.def
  clang/include/clang/Basic/TargetInfo.h
  clang/include/clang/Driver/Options.td
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/ExprConstant.cpp
  clang/lib/Basic/TargetInfo.cpp
  clang/lib/Basic/Targets/AMDGPU.cpp
  clang/lib/Basic/Targets/AMDGPU.h
  clang/lib/Basic/Targets/PPC.cpp
  clang/lib/Basic/Targets/PPC.h
  clang/lib/Basic/Targets/SPIR.h
  clang/lib/Basic/Targets/WebAssembly.cpp
  clang/lib/Basic/Targets/WebAssembly.h
  clang/lib/Basic/Targets/X86.h
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/ASTUnit.cpp
  clang/lib/Frontend/CompilerInstance.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/lib/Sema/SemaCoroutine.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/test/CodeGen/arithmetic-fence-builtin.c
  clang/test/Driver/clang_f_opts.c
  clang/test/Sema/arithmetic-fence-builtin.c
  clang/tools/clang-import-test/clang-import-test.cpp

Index: clang/tools/clang-import-test/clang-import-test.cpp
===
--- clang/tools/clang-import-test/clang-import-test.cpp
+++ clang/tools/clang-import-test/clang-import-test.cpp
@@ -208,7 +208,7 @@
   TargetInfo *TI = TargetInfo::CreateTargetInfo(
   Ins->getDiagnostics(), Ins->getInvocation().TargetOpts);
   Ins->setTarget(TI);
-  Ins->getTarget().adjust(Ins->getLangOpts());
+  Ins->getTarget().adjust(Ins->getDiagnostics(), Ins->getLangOpts());
   Ins->createFileManager();
   Ins->createSourceManager(Ins->getFileManager());
   Ins->createPreprocessor(TU_Complete);
Index: clang/test/Sema/arithmetic-fence-builtin.c
===
--- /dev/null
+++ clang/test/Sema/arithmetic-fence-builtin.c
@@ -0,0 +1,45 @@
+// RUN: %clang_cc1 -triple i386-pc-linux-gnu -emit-llvm -o - -verify -x c++ %s
+// RUN: %clang_cc1 -triple ppc64le -DPPC -emit-llvm -o - -verify -x c++ %s
+// RUN: not %clang_cc1 -triple ppc64le -DPPC -emit-llvm -o - -x c++ %s \
+// RUN:-fprotect-parens 2>&1 | FileCheck -check-prefix=PPC %s
+#ifndef PPC
+int v;
+template  T addT(T a, T b) {
+  T *q = __arithmetic_fence();
+  // expected-error@-1 {{operand of type 'float *' where floating, complex or a vector of such types is required ('float *' invalid)}}
+  // expected-error@-2 {{operand of type 'int *' where floating, complex or a vector of such types is required ('int *' invalid)}}
+  return __arithmetic_fence(a + b);
+  // expected-error@-1 {{operand of type 'int' where floating, complex or a vector of such types is required ('int' invalid)}}
+}
+int addit(int a, int b) {
+  float x, y;
+  typedef struct {
+int a, b;
+  } stype;
+  stype s;
+  s = __arithmetic_fence(s);// expected-error {{operand of type 'stype' where floating, complex or a vector of such types is required ('stype' invalid)}}
+  x = __arithmetic_fence(x, y); // expected-error {{too many arguments to function call, expected 1, have 2}}
+  // Complex is supported.
+  _Complex double cd, cd1;
+  cd = __arithmetic_fence(cd1);
+  // Vector is supported.
+  typedef float __v4hi __attribute__((__vector_size__(8)));
+  __v4hi vec1, vec2;
+  vec1 = __arithmetic_fence(vec2);
+
+  v = __arithmetic_fence(a + b); // expected-error {{operand of type 'int' where floating, complex or a vector of such types is required ('int' invalid)}}
+  float f = addT(a, b);   // expected-note {{in instantiation of function template specialization 'addT' requested here}}
+  int i = addT(1, 2);   // expected-note {{in instantiation of function template specialization 'addT' requested here}}
+  constexpr float d = 1.0 + 2.0;
+  constexpr float c = __arithmetic_fence(1.0 + 2.0);
+  // expected-error@-1 {{constexpr variable 'c' must be initialized by a constant expression}}
+  return 0;
+}
+// expected-error@+1 {{static_assert expression is not an integral constant expression}}
+static_assert( __arithmetic_fence(1.0 + 2.0), "message" );
+#else
+float addit(float a, float b) {
+  return __arithmetic_fence(a+b); // expected-error {{builtin is not supported on this target}}
+}
+#endif
+//PPC: error: option '-fprotect-parens' cannot be specified on this target
Index: clang/test/Driver/clang_f_opts.c

[PATCH] D100118: [clang] RFC Support new builtin __arithmetic_fence to control floating point optimization, and new clang option fprotect-parens

2021-04-21 Thread Melanie Blower via Phabricator via cfe-commits 
mibintc added inline comments.



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2844
+  return RValue::get(
+  Builder.CreateArithmeticFence(ArgValue, ConvertType(ArgType)));
+return RValue::get(ArgValue);

mibintc wrote:
> mibintc wrote:
> > kpn wrote:
> > > Does this say that the fence will be silently dropped if any of the fast 
> > > math flags are disabled? Silently dropping something used for correctness 
> > > makes me nervous. Is there a case where all of these flags are required 
> > > for correct behavior of the fence?
> > Yes that is the idea, the clang builtin is only meaningful for float 
> > operations when -ffast-math is enabled.  If fast math isn't enabled then 
> > the operations should already be performed strictly.  I didn't have 
> > "isFast", should rewrite with isFast to make it easier to understand 
> Oops I'm wrong. @kbsmith1 tells me only the reassoc bit is interesting. i'll 
> need to fix this in the documentation etc. Thanks for your careful reading. 
BTW in the options parsing, any of the unsafe fp math options freciprocal-math, 
fsigned-zeros and fapprox-func have the effect of enabing 
LangOptions.AllowFPReassoc.  This is in the file Options.td


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100118

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100118: [clang] RFC Support new builtin __arithmetic_fence to control floating point optimization, and new clang option fprotect-parens

2021-04-20 Thread Melanie Blower via Phabricator via cfe-commits 
mibintc added inline comments.



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2844
+  return RValue::get(
+  Builder.CreateArithmeticFence(ArgValue, ConvertType(ArgType)));
+return RValue::get(ArgValue);

mibintc wrote:
> kpn wrote:
> > Does this say that the fence will be silently dropped if any of the fast 
> > math flags are disabled? Silently dropping something used for correctness 
> > makes me nervous. Is there a case where all of these flags are required for 
> > correct behavior of the fence?
> Yes that is the idea, the clang builtin is only meaningful for float 
> operations when -ffast-math is enabled.  If fast math isn't enabled then the 
> operations should already be performed strictly.  I didn't have "isFast", 
> should rewrite with isFast to make it easier to understand 
Oops I'm wrong. @kbsmith1 tells me only the reassoc bit is interesting. i'll 
need to fix this in the documentation etc. Thanks for your careful reading. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100118

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100118: [clang] RFC Support new builtin __arithmetic_fence to control floating point optimization, and new clang option fprotect-parens

2021-04-20 Thread Melanie Blower via Phabricator via cfe-commits 
mibintc added inline comments.



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2844
+  return RValue::get(
+  Builder.CreateArithmeticFence(ArgValue, ConvertType(ArgType)));
+return RValue::get(ArgValue);

kpn wrote:
> Does this say that the fence will be silently dropped if any of the fast math 
> flags are disabled? Silently dropping something used for correctness makes me 
> nervous. Is there a case where all of these flags are required for correct 
> behavior of the fence?
Yes that is the idea, the clang builtin is only meaningful for float operations 
when -ffast-math is enabled.  If fast math isn't enabled then the operations 
should already be performed strictly.  I didn't have "isFast", should rewrite 
with isFast to make it easier to understand 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100118

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100118: [clang] RFC Support new builtin __arithmetic_fence to control floating point optimization, and new clang option fprotect-parens

2021-04-20 Thread Kevin P. Neal via Phabricator via cfe-commits 
kpn added inline comments.



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2844
+  return RValue::get(
+  Builder.CreateArithmeticFence(ArgValue, ConvertType(ArgType)));
+return RValue::get(ArgValue);

Does this say that the fence will be silently dropped if any of the fast math 
flags are disabled? Silently dropping something used for correctness makes me 
nervous. Is there a case where all of these flags are required for correct 
behavior of the fence?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100118

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100118: [clang] RFC Support new builtin __arithmetic_fence to control floating point optimization, and new clang option fprotect-parens

2021-04-16 Thread Kevin P. Neal via Phabricator via cfe-commits 
kpn added a comment.

In D100118#2695365 , @kpn wrote:

> I thought that adding a new fence required changing every optimization pass 
> in LLVM. That's why the constrained intrinsics were implemented they way they 
> are where no fence is needed.
>
> Aren't you going to have miscompiles using this new fence until all that 
> optimization work is done? Or am I wrong? @andrew.w.kaylor?

Perhaps there is no problem. I'm looking at D99675 
 now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100118

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100118: [clang] RFC Support new builtin __arithmetic_fence to control floating point optimization, and new clang option fprotect-parens

2021-04-16 Thread Kevin P. Neal via Phabricator via cfe-commits 
kpn added a comment.

I thought that adding a new fence required changing every optimization pass in 
LLVM. That's why the constrained intrinsics were implemented they way they are 
where no fence is needed.

Aren't you going to have miscompiles using this new fence until all that 
optimization work is done? Or am I wrong? @andrew.w.kaylor?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100118

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100118: [clang] RFC Support new builtin __arithmetic_fence to control floating point optimization, and new clang option fprotect-parens

2021-04-15 Thread Melanie Blower via Phabricator via cfe-commits 
mibintc updated this revision to Diff 337881.
mibintc retitled this revision from "[clang] RFC Support new builtin 
__arithmetic_fence to control floating point optiization" to "[clang] RFC 
Support new builtin __arithmetic_fence to control floating point optimization, 
and new clang option fprotect-parens".
mibintc added a comment.

This is a minor change with only formatting changes, this patch is not yet 
ready for review, only discussion. 
Together with the llvm parent patch, this simple program can now run end-to-end

  clang -c -ffast-math test.c
  
  float addF(float x, float y) {
return __arithmetic_fence(x + y);
  }


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100118

Files:
  clang/include/clang/Basic/Builtins.def
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/ExprConstant.cpp
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/test/CodeGen/arithmetic-fence-builtin.c
  clang/test/Sema/arithmetic-fence-builtin.c

Index: clang/test/Sema/arithmetic-fence-builtin.c
===
--- /dev/null
+++ clang/test/Sema/arithmetic-fence-builtin.c
@@ -0,0 +1,33 @@
+// RUN: %clang_cc1 -triple i386-pc-linux-gnu -emit-llvm -o - -verify -x c++ %s
+int v;
+template  T addT(T a, T b) {
+  T *q = __arithmetic_fence();
+  // expected-error@-1 {{operand of type 'float *' where floating, complex or a vector of such types is required ('float *' invalid)}}
+  // expected-error@-2 {{operand of type 'int *' where floating, complex or a vector of such types is required ('int *' invalid)}}
+  return __arithmetic_fence(a + b);
+  // expected-error@-1 {{operand of type 'int' where floating, complex or a vector of such types is required ('int' invalid)}}
+}
+int addit(int a, int b) {
+  float x, y;
+  typedef struct {
+int a, b;
+  } stype;
+  stype s;
+  s = __arithmetic_fence(s);// expected-error {{operand of type 'stype' where floating, complex or a vector of such types is required ('stype' invalid)}}
+  x = __arithmetic_fence(x, y); // expected-error {{too many arguments to function call, expected 1, have 2}}
+  // Complex is supported.
+  _Complex double cd, cd1;
+  cd = __arithmetic_fence(cd1);
+  // Vector is supported.
+  typedef float __v4hi __attribute__((__vector_size__(8)));
+  __v4hi vec1, vec2;
+  vec1 = __arithmetic_fence(vec2);
+
+  v = __arithmetic_fence(a + b); // expected-error {{operand of type 'int' where floating, complex or a vector of such types is required ('int' invalid)}}
+  float f = addT(a, b);   // expected-note {{in instantiation of function template specialization 'addT' requested here}}
+  int i = addT(1, 2);   // expected-note {{in instantiation of function template specialization 'addT' requested here}}
+  constexpr float d = 1.0 + 2.0;
+  constexpr float c = __arithmetic_fence(1.0 + 2.0);
+  // expected-error@-1 {{constexpr variable 'c' must be initialized by a constant expression}}
+  return 0;
+}
Index: clang/test/CodeGen/arithmetic-fence-builtin.c
===
--- /dev/null
+++ clang/test/CodeGen/arithmetic-fence-builtin.c
@@ -0,0 +1,35 @@
+// Test with fast math
+// RUN: %clang_cc1 -triple i386-pc-linux-gnu -emit-llvm \
+// RUN: -menable-no-infs -menable-no-nans -menable-unsafe-fp-math \
+// RUN: -fno-signed-zeros -mreassociate -freciprocal-math \
+// RUN: -ffp-contract=fast -fno-rounding-math -ffast-math -ffinite-math-only \
+// RUN: -o - %s | FileCheck %s
+//
+// Test with fast math, showing incomplete implementaton for Complex
+// this test fails.
+// RUN: %clang_cc1 -triple i386-pc-linux-gnu -emit-llvm \
+// RUN: -menable-no-infs -menable-no-nans -menable-unsafe-fp-math \
+// RUN: -fno-signed-zeros -mreassociate -freciprocal-math \
+// RUN: -ffp-contract=fast -fno-rounding-math -ffast-math -ffinite-math-only \
+// RUN: -o - -DSHOWBUG %s | FileCheck %s
+//
+// TBD: Add test without fast flags showing llvm intrinsic not created
+int v;
+int addit(float a, float b) {
+//CHECK: define {{.*}} @addit(float %a, float %b) #0 {
+#ifdef SHOWBUG
+  // Assertion fail in clang when try to Emit complex expression
+  // Complex should be supported.
+  _Complex double cd, cd1;
+  cd = __arithmetic_fence(cd1);
+#endif
+  // Vector should be supported.
+  typedef float __v2f32 __attribute__((__vector_size__(8)));
+  __v2f32 vec1, vec2;
+  vec1 = __arithmetic_fence(vec2);
+  // CHECK: call fast <2 x float> @llvm.arithmetic.fence.v2f32
+
+  v = __arithmetic_fence(a + b);
+  // CHECK: call fast float @llvm.arithmetic.fence.f32(float %add)
+  return 0;
+}
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -1555,6 +1555,10 @@
 Diag(TheCall->getBeginLoc(), diag::warn_alloca)
 <<