[PATCH] D64294: [Driver] Consolidate shouldUseFramePointer() and shouldUseLeafFramePointer()

[Fangrui Song via Phabricator via cfe-commits]

MaskRay updated this revision to Diff 208289.
MaskRay added a comment.

Simplify


Repository:
  rC Clang

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

https://reviews.llvm.org/D64294

Files:
  lib/Driver/ToolChains/Clang.cpp
  test/Driver/cl-options.c
  test/Driver/frame-pointer-elim.c
  test/Driver/xcore-opts.c

Index: test/Driver/xcore-opts.c
===
--- test/Driver/xcore-opts.c
+++ test/Driver/xcore-opts.c
@@ -4,8 +4,8 @@
 // RUN: %clang -target xcore %s -g0 -### -o %t.o 2>&1 | FileCheck -check-prefix CHECK-G0 %s
 
 // CHECK: "-nostdsysteminc"
-// CHECK: "-momit-leaf-frame-pointer"
 // CHECK-NOT: "-mdisable-fp-elim"
+// CHECK-NOT: "-momit-leaf-frame-pointer"
 // CHECK: "-fno-signed-char"
 // CHECK: "-fno-use-cxa-atexit"
 // CHECK-NOT: "-fcxx-exceptions"
Index: test/Driver/frame-pointer-elim.c
===
--- test/Driver/frame-pointer-elim.c
+++ test/Driver/frame-pointer-elim.c
@@ -1,48 +1,50 @@
+// KEEP-ALL: "-mdisable-fp-elim"
+// KEEP-ALL-NOT: "-momit-leaf-frame-pointer"
+
+// KEEP-NON-LEAF: "-mdisable-fp-elim"
+// KEEP-NON-LEAF: "-momit-leaf-frame-pointer"
+
+// KEEP-NONE-NOT: "-mdisable-fp-elim"
+// KEEP-NONE-NOT: "-momit-leaf-frame-pointer"
+
 // For these next two tests when optimized we should omit the leaf frame
 // pointer, for unoptimized we should have a leaf frame pointer.
 // RUN: %clang -### -target i386-pc-linux-gnu -S -O1 %s 2>&1 | \
-// RUN:   FileCheck --check-prefix=LINUX-OPT %s
-// LINUX-OPT: "-momit-leaf-frame-pointer"
+// RUN:   FileCheck --check-prefix=KEEP-NONE %s
 
 // RUN: %clang -### -target i386-pc-linux-gnu -S %s 2>&1 | \
-// RUN:   FileCheck --check-prefix=LINUX %s
-// LINUX-NOT: "-momit-leaf-frame-pointer"
+// RUN:   FileCheck --check-prefix=KEEP-ALL %s
+
+// RUN: %clang -### -target i386-pc-linux-gnu -S -O1 -fno-omit-frame-pointer %s 2>&1 | \
+// RUN:   FileCheck --check-prefix=KEEP-ALL %s
 
 // CloudABI follows the same rules as Linux.
 // RUN: %clang -### -target x86_64-unknown-cloudabi -S -O1 %s 2>&1 | \
-// RUN:   FileCheck --check-prefix=CLOUDABI-OPT %s
-// CLOUDABI-OPT: "-momit-leaf-frame-pointer"
+// RUN:   FileCheck --check-prefix=KEEP-NONE %s
 
 // RUN: %clang -### -target x86_64-unknown-cloudabi -S %s 2>&1 | \
-// RUN:   FileCheck --check-prefix=CLOUDABI %s
-// CLOUDABI-NOT: "-momit-leaf-frame-pointer"
+// RUN:   FileCheck --check-prefix=KEEP-ALL %s
 
 // NetBSD follows the same rules as Linux.
 // RUN: %clang -### -target x86_64-unknown-netbsd -S -O1 %s 2>&1 | \
-// RUN:   FileCheck --check-prefix=NETBSD-OPT %s
-// NETBSD-OPT: "-momit-leaf-frame-pointer"
+// RUN:   FileCheck --check-prefix=KEEP-NONE %s
 
 // RUN: %clang -### -target x86_64-unknown-netbsd -S %s 2>&1 | \
-// RUN:   FileCheck --check-prefix=NETBSD %s
-// NETBSD-NOT: "-momit-leaf-frame-pointer"
+// RUN:   FileCheck --check-prefix=KEEP-ALL %s
 
 // Darwin disables omitting the leaf frame pointer even under optimization
 // unless the command lines are given.
 // RUN: %clang -### -target i386-apple-darwin -S %s 2>&1 | \
-// RUN:   FileCheck --check-prefix=DARWIN %s
-// DARWIN: "-mdisable-fp-elim"
+// RUN:   FileCheck --check-prefix=KEEP-ALL %s
 
 // RUN: %clang -### -target i386-apple-darwin -S -O1 %s 2>&1 | \
-// RUN:   FileCheck --check-prefix=DARWIN-OPT %s
-// DARWIN-OPT-NOT: "-momit-leaf-frame-pointer"
+// RUN:   FileCheck --check-prefix=KEEP-ALL %s
 
 // RUN: %clang -### -target i386-darwin -S -fomit-frame-pointer %s 2>&1 | \
-// RUN:   FileCheck --check-prefix=OMIT_ALL %s
-// OMIT_ALL-NOT: "-mdisable-fp-elim"
+// RUN:   FileCheck --check-prefix=KEEP-NONE %s
 
 // RUN: %clang -### -target i386-darwin -S -momit-leaf-frame-pointer %s 2>&1 | \
-// RUN:   FileCheck --check-prefix=OMIT_LEAF %s
-// OMIT_LEAF: "-momit-leaf-frame-pointer"
+// RUN:   FileCheck --check-prefix=KEEP-NON-LEAF %s
 
 // RUN: %clang -### -target armv7s-apple-ios -fomit-frame-pointer %s 2>&1 | \
 // RUN:   FileCheck --check-prefix=WARN-OMIT-7S %s
@@ -63,11 +65,10 @@
 // WARN-OMIT-LEAF-7S: "-momit-leaf-frame-pointer"
 
 // On the PS4, we default to omitting the frame pointer on leaf functions
-// (OMIT_LEAF check line is above)
 // RUN: %clang -### -target x86_64-scei-ps4 -S %s 2>&1 | \
-// RUN:   FileCheck --check-prefix=OMIT_LEAF %s
+// RUN:   FileCheck --check-prefix=KEEP-NON-LEAF %s
 // RUN: %clang -### -target x86_64-scei-ps4 -S -O2 %s 2>&1 | \
-// RUN:   FileCheck --check-prefix=OMIT_LEAF %s
+// RUN:   FileCheck --check-prefix=KEEP-NON-LEAF %s
 
 void f0() {}
 void f1() { f0(); }
Index: test/Driver/cl-options.c
===
--- test/Driver/cl-options.c
+++ test/Driver/cl-options.c
@@ -161,28 +161,28 @@
 // RUN: %clang_cl /Os --target=i686-pc-windows-msvc -### -- %s 2>&1 | FileCheck -check-prefix=Os %s
 // RUN: %clang_cl /Os --target=x86_64-pc-windows-msvc -### -- %s 2>&1 | FileCheck -check-prefix=Os %s
 // Os-NOT: -mdisable-fp-elim
-/

[PATCH] D64294: [Driver] Consolidate shouldUseFramePointer() and shouldUseLeafFramePointer()

[Fangrui Song via Phabricator via cfe-commits]

MaskRay created this revision.
MaskRay added reviewers: chandlerc, dim, echristo, rnk, rsmith, void, ychen.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Use a tri-state enum to represent shouldUseFramePointer() and
shouldUseLeafFramePointer().

This simplifies the logic and fixes PR9825:

  need both -fno-omit-frame-pointer and -mno-omit-leaf-frame-pointer to
  generate fp on a leaf function.

and PR24003:

  /Oy- /O2 should not omit leaf frame pointer.

and:

  when CC1 option -mdisable-fp-elim if absent, -momit-leaf-frame-pointer
  can also be omitted.


Repository:
  rC Clang

https://reviews.llvm.org/D64294

Files:
  lib/Driver/ToolChains/Clang.cpp
  test/Driver/cl-options.c
  test/Driver/frame-pointer-elim.c
  test/Driver/xcore-opts.c

Index: test/Driver/xcore-opts.c
===
--- test/Driver/xcore-opts.c
+++ test/Driver/xcore-opts.c
@@ -4,8 +4,8 @@
 // RUN: %clang -target xcore %s -g0 -### -o %t.o 2>&1 | FileCheck -check-prefix CHECK-G0 %s
 
 // CHECK: "-nostdsysteminc"
-// CHECK: "-momit-leaf-frame-pointer"
 // CHECK-NOT: "-mdisable-fp-elim"
+// CHECK-NOT: "-momit-leaf-frame-pointer"
 // CHECK: "-fno-signed-char"
 // CHECK: "-fno-use-cxa-atexit"
 // CHECK-NOT: "-fcxx-exceptions"
Index: test/Driver/frame-pointer-elim.c
===
--- test/Driver/frame-pointer-elim.c
+++ test/Driver/frame-pointer-elim.c
@@ -1,48 +1,50 @@
+// KEEP-ALL: "-mdisable-fp-elim"
+// KEEP-ALL-NOT: "-momit-leaf-frame-pointer"
+
+// KEEP-NON-LEAF: "-mdisable-fp-elim"
+// KEEP-NON-LEAF: "-momit-leaf-frame-pointer"
+
+// KEEP-NONE-NOT: "-mdisable-fp-elim"
+// KEEP-NONE-NOT: "-momit-leaf-frame-pointer"
+
 // For these next two tests when optimized we should omit the leaf frame
 // pointer, for unoptimized we should have a leaf frame pointer.
 // RUN: %clang -### -target i386-pc-linux-gnu -S -O1 %s 2>&1 | \
-// RUN:   FileCheck --check-prefix=LINUX-OPT %s
-// LINUX-OPT: "-momit-leaf-frame-pointer"
+// RUN:   FileCheck --check-prefix=KEEP-NONE %s
 
 // RUN: %clang -### -target i386-pc-linux-gnu -S %s 2>&1 | \
-// RUN:   FileCheck --check-prefix=LINUX %s
-// LINUX-NOT: "-momit-leaf-frame-pointer"
+// RUN:   FileCheck --check-prefix=KEEP-ALL %s
+
+// RUN: %clang -### -target i386-pc-linux-gnu -S -O1 -fno-omit-frame-pointer %s 2>&1 | \
+// RUN:   FileCheck --check-prefix=KEEP-ALL %s
 
 // CloudABI follows the same rules as Linux.
 // RUN: %clang -### -target x86_64-unknown-cloudabi -S -O1 %s 2>&1 | \
-// RUN:   FileCheck --check-prefix=CLOUDABI-OPT %s
-// CLOUDABI-OPT: "-momit-leaf-frame-pointer"
+// RUN:   FileCheck --check-prefix=KEEP-NONE %s
 
 // RUN: %clang -### -target x86_64-unknown-cloudabi -S %s 2>&1 | \
-// RUN:   FileCheck --check-prefix=CLOUDABI %s
-// CLOUDABI-NOT: "-momit-leaf-frame-pointer"
+// RUN:   FileCheck --check-prefix=KEEP-ALL %s
 
 // NetBSD follows the same rules as Linux.
 // RUN: %clang -### -target x86_64-unknown-netbsd -S -O1 %s 2>&1 | \
-// RUN:   FileCheck --check-prefix=NETBSD-OPT %s
-// NETBSD-OPT: "-momit-leaf-frame-pointer"
+// RUN:   FileCheck --check-prefix=KEEP-NONE %s
 
 // RUN: %clang -### -target x86_64-unknown-netbsd -S %s 2>&1 | \
-// RUN:   FileCheck --check-prefix=NETBSD %s
-// NETBSD-NOT: "-momit-leaf-frame-pointer"
+// RUN:   FileCheck --check-prefix=KEEP-ALL %s
 
 // Darwin disables omitting the leaf frame pointer even under optimization
 // unless the command lines are given.
 // RUN: %clang -### -target i386-apple-darwin -S %s 2>&1 | \
-// RUN:   FileCheck --check-prefix=DARWIN %s
-// DARWIN: "-mdisable-fp-elim"
+// RUN:   FileCheck --check-prefix=KEEP-ALL %s
 
 // RUN: %clang -### -target i386-apple-darwin -S -O1 %s 2>&1 | \
-// RUN:   FileCheck --check-prefix=DARWIN-OPT %s
-// DARWIN-OPT-NOT: "-momit-leaf-frame-pointer"
+// RUN:   FileCheck --check-prefix=KEEP-ALL %s
 
 // RUN: %clang -### -target i386-darwin -S -fomit-frame-pointer %s 2>&1 | \
-// RUN:   FileCheck --check-prefix=OMIT_ALL %s
-// OMIT_ALL-NOT: "-mdisable-fp-elim"
+// RUN:   FileCheck --check-prefix=KEEP-NONE %s
 
 // RUN: %clang -### -target i386-darwin -S -momit-leaf-frame-pointer %s 2>&1 | \
-// RUN:   FileCheck --check-prefix=OMIT_LEAF %s
-// OMIT_LEAF: "-momit-leaf-frame-pointer"
+// RUN:   FileCheck --check-prefix=KEEP-NON-LEAF %s
 
 // RUN: %clang -### -target armv7s-apple-ios -fomit-frame-pointer %s 2>&1 | \
 // RUN:   FileCheck --check-prefix=WARN-OMIT-7S %s
@@ -63,11 +65,10 @@
 // WARN-OMIT-LEAF-7S: "-momit-leaf-frame-pointer"
 
 // On the PS4, we default to omitting the frame pointer on leaf functions
-// (OMIT_LEAF check line is above)
 // RUN: %clang -### -target x86_64-scei-ps4 -S %s 2>&1 | \
-// RUN:   FileCheck --check-prefix=OMIT_LEAF %s
+// RUN:   FileCheck --check-prefix=KEEP-NON-LEAF %s
 // RUN: %clang -### -target x86_64-scei-ps4 -S -O2 %s 2>&1 | \
-// RUN:   FileCheck --check-prefix=OMIT_LEAF %s
+// RUN:   FileCheck --check-prefix=KEEP-NON-LEAF %s
 
 void f0()

[PATCH] D64289: [OpenMP] Fix 128-bit long double support on target

[Joel E. Denny via Phabricator via cfe-commits]

jdenny updated this revision to Diff 208286.
jdenny added a comment.

This incorporates all the improvements I think we've discussed so far:

- In diagnostics, distinguish between unsupported types and non-equivalent 
types.
- Don't treat `__float128` and `long double` as equivalent.
- Compare exact floating point representation instead of just size.


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

https://reviews.llvm.org/D64289

Files:
  clang/include/clang/AST/ASTContext.h
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/AST/ASTContext.cpp
  clang/lib/Sema/SemaOpenMP.cpp
  clang/test/OpenMP/nvptx_unsupported_type_messages.cpp

Index: clang/test/OpenMP/nvptx_unsupported_type_messages.cpp
===
--- clang/test/OpenMP/nvptx_unsupported_type_messages.cpp
+++ clang/test/OpenMP/nvptx_unsupported_type_messages.cpp
@@ -1,8 +1,16 @@
 // Test target codegen - host bc file has to be created first.
-// RUN: %clang_cc1 -fopenmp -x c++ -triple x86_64-unknown-linux -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm-bc %s -o %t-host.bc
+// RUN: %clang_cc1 -verify=quiet -fopenmp -x c++ -triple x86_64-unknown-linux -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm-bc %s -o %t-host.bc
 // RUN: %clang_cc1 -verify -fopenmp -x c++ -triple nvptx64-unknown-unknown -aux-triple x86_64-unknown-linux -fopenmp-targets=nvptx64-nvidia-cuda %s -fopenmp-is-device -fopenmp-host-ir-file-path %t-host.bc -fsyntax-only
-// RUN: %clang_cc1 -fopenmp -x c++ -triple powerpc64le-unknown-linux-gnu -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm-bc %s -o %t-host.bc
+// RUN: %clang_cc1 -verify=quiet -fopenmp -x c++ -triple powerpc64le-unknown-linux-gnu -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm-bc %s -o %t-host.bc
 // RUN: %clang_cc1 -verify -fopenmp -x c++ -triple nvptx64-unknown-unknown -aux-triple powerpc64le-unknown-linux-gnu -fopenmp-targets=nvptx64-nvidia-cuda %s -fopenmp-is-device -fopenmp-host-ir-file-path %t-host.bc -fsyntax-only
+//
+// Make sure this code does compile when the host and target are the same.
+// RUN: %clang_cc1 -verify=quiet -fopenmp -x c++ -triple x86_64-unknown-linux -fopenmp-targets=x86_64-unknown-linux -emit-llvm-bc %s -o %t-host.bc
+// RUN: %clang_cc1 -verify=quiet -fopenmp -x c++ -triple x86_64-unknown-linux -aux-triple x86_64-unknown-linux -fopenmp-targets=x86_64-unknown-linux %s -fopenmp-is-device -fopenmp-host-ir-file-path %t-host.bc -fsyntax-only
+// RUN: %clang_cc1 -verify=quiet -fopenmp -x c++ -triple powerpc64le-unknown-linux-gnu -fopenmp-targets=powerpc64le-unknown-linux-gnu -emit-llvm-bc %s -o %t-host.bc
+// RUN: %clang_cc1 -verify=quiet -fopenmp -x c++ -triple powerpc64le-unknown-linux-gnu -aux-triple powerpc64le-unknown-linux-gnu -fopenmp-targets=powerpc64le-unknown-linux-gnu %s -fopenmp-is-device -fopenmp-host-ir-file-path %t-host.bc -fsyntax-only
+
+// quiet-no-diagnostics
 
 struct T {
   char a;
@@ -16,7 +24,7 @@
 #ifndef _ARCH_PPC
 // expected-error@+4 {{'__float128' is not supported on this target}}
 #else
-// expected-error@+2 {{'long double' is not supported on this target}}
+// expected-error@+2 {{'long double' is not equivalent between host and target}}
 #endif
   T &operator+(T &b) { f += b.a; return *this;}
 };
Index: clang/lib/Sema/SemaOpenMP.cpp
===
--- clang/lib/Sema/SemaOpenMP.cpp
+++ clang/lib/Sema/SemaOpenMP.cpp
@@ -1588,13 +1588,23 @@
  "OpenMP device compilation mode is expected.");
   QualType Ty = E->getType();
   if ((Ty->isFloat16Type() && !Context.getTargetInfo().hasFloat16Type()) ||
-  ((Ty->isFloat128Type() ||
-(Ty->isRealFloatingType() && Context.getTypeSize(Ty) == 128)) &&
-   !Context.getTargetInfo().hasFloat128Type()) ||
+  (Ty->isFloat128Type() && !Context.getTargetInfo().hasFloat128Type()) ||
   (Ty->isIntegerType() && Context.getTypeSize(Ty) == 128 &&
!Context.getTargetInfo().hasInt128Type()))
 targetDiag(E->getExprLoc(), diag::err_type_unsupported)
 << Ty << E->getSourceRange();
+  if (Ty->isRealFloatingType()) {
+llvm::APFloatBase::Semantics Sem = llvm::APFloatBase::SemanticsToEnum(
+Context.getFloatTypeSemantics(Ty, /*PreferAuxTarget*/ true));
+const TargetInfo &TI = Context.getTargetInfo();
+if ((Ty->isFloat128Type() &&
+ Sem != llvm::APFloatBase::SemanticsToEnum(TI.getFloat128Format())) ||
+(Ty.getUnqualifiedType() == Context.LongDoubleTy &&
+ Sem != llvm::APFloatBase::SemanticsToEnum(TI.getLongDoubleFormat(
+  // TODO: Naming the floating point representations would be helpful.
+  targetDiag(E->getExprLoc(), diag::err_type_not_equivalent)
+  << Ty << E->getSourceRange();
+  }
 }
 
 bool Sema::isOpenMPCapturedByRef(const ValueDecl *D, unsigned Level) const {
Index: clang/lib/AST/ASTContext.cpp
===
--- clang/lib/AST/ASTContext.cpp
+++ cl

[PATCH] D10049: Driver: Add OS-specific resource directory to file search paths.

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

Implemented by D31447 


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

https://reviews.llvm.org/D10049



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

[PATCH] D10051: Driver: Modify the -print-libgcc-file-name code to use ToolChain::GetRuntimeLibPath().

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

D25338  fixed the issue..


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

https://reviews.llvm.org/D10051



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

[PATCH] D64289: [OpenMP] Fix 128-bit long double support on target

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added inline comments.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:1594
+   !Context.getTargetInfo().hasFloat128Type() &&
+   Context.getTargetInfo().getLongDoubleWidth() != 128) ||
   (Ty->isIntegerType() && Context.getTypeSize(Ty) == 128 &&

ABataev wrote:
> jdenny wrote:
> > ABataev wrote:
> > > MaskRay wrote:
> > > > ABataev wrote:
> > > > > jdenny wrote:
> > > > > > ABataev wrote:
> > > > > > > jdenny wrote:
> > > > > > > > ABataev wrote:
> > > > > > > > > jdenny wrote:
> > > > > > > > > > ABataev wrote:
> > > > > > > > > > > jdenny wrote:
> > > > > > > > > > > > ABataev wrote:
> > > > > > > > > > > > > jdenny wrote:
> > > > > > > > > > > > > > ABataev wrote:
> > > > > > > > > > > > > > > Hmm, this look strange, at least. Seems to me, in 
> > > > > > > > > > > > > > > this case the size of the long double is 128 bit 
> > > > > > > > > > > > > > > (copied from the host), but device reports that 
> > > > > > > > > > > > > > > it does not support 128 bit double. Seems to me, 
> > > > > > > > > > > > > > > it is a problem with the device configuration. 
> > > > > > > > > > > > > > > Why does the host translate long double to 128 
> > > > > > > > > > > > > > > bit fp, while the device translates it to 64 bit 
> > > > > > > > > > > > > > > FP?
> > > > > > > > > > > > > > Sorry, I think I've misunderstood what's happening 
> > > > > > > > > > > > > > here, and my fix is probably wrong.
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > For x86_64, the example from my patch summary fails 
> > > > > > > > > > > > > > as described there.  Does that work for you?
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > For powerpc64le, the reproducer I added to the test 
> > > > > > > > > > > > > > suite fails without this patch.  Shouldn't it 
> > > > > > > > > > > > > > succeed?
> > > > > > > > > > > > > Still, seems to me like the problem with the device 
> > > > > > > > > > > > > config, not the original check.
> > > > > > > > > > > > > Still, seems to me like the problem with the device 
> > > > > > > > > > > > > config, not the original check.
> > > > > > > > > > > > 
> > > > > > > > > > > > I'm not sure where to begin looking for that.  Can you 
> > > > > > > > > > > > point me in the right direction?  Thanks.
> > > > > > > > > > > You need to understand why host and device report 
> > > > > > > > > > > different size of the type. Check how the device is 
> > > > > > > > > > > configured in lib/Basic/Targets
> > > > > > > > > > Thanks for the pointer.  I think I understand things a bit 
> > > > > > > > > > better now.
> > > > > > > > > > 
> > > > > > > > > > Without this patch's fix, the x86_64 example from this 
> > > > > > > > > > patch's summary fails while this patch's new x86_64 test 
> > > > > > > > > > case passes.  The difference is the summary's example 
> > > > > > > > > > doesn't specify `-unknown-linux` after `x86_64`, and that's 
> > > > > > > > > > what sets `hasFloat128Type()` to true.
> > > > > > > > > > 
> > > > > > > > > > `powerpc64le-unknown-linux-gnu` does not have `__float128`, 
> > > > > > > > > > it seems.  That's why this patch's new powerpc64le test 
> > > > > > > > > > case fails without this patch's fix.
> > > > > > > > > > 
> > > > > > > > > > It seems strange to me that the code we're commenting on 
> > > > > > > > > > originally looks for the source type to be either 
> > > > > > > > > > `__float128` or 128-bit `long double`, and it then requires 
> > > > > > > > > > the target to support `__float128`.  It doesn't accept 
> > > > > > > > > > 128-bit `long double` support as sufficient.  My intention 
> > > > > > > > > > in this patch was to extend it to accept either so that all 
> > > > > > > > > > the examples above compile.  Is that too lenient?  Am I 
> > > > > > > > > > misinterpreting what's happening?
> > > > > > > > > > 
> > > > > > > > > > As for your comment about 64-bit floating point in the 
> > > > > > > > > > device translation, I haven't seen that yet.  Did I miss it?
> > > > > > > > > The intention of the original patch is to make host and 
> > > > > > > > > device to have the same float128 and long double types. 
> > > > > > > > > Device inherits those types from the host to be compatible 
> > > > > > > > > during offloading and to correctly mangle functions.
> > > > > > > > > Without this we just can't generate offloading regions 
> > > > > > > > > correctly. If the host has 128 bit long double, the device 
> > > > > > > > > also must have 128 bit long double. 
> > > > > > > > > If device does not support 128bit floats, in this case device 
> > > > > > > > > can only move the data (do load/stores ops only) and cannot 
> > > > > > > > > do anything else.
> > > > > > > > Are you intentionally requiring support for `__float128` when 
> > > > > > > > the source type is 128-bit `long double`?  That seems to mean 
> > > > > > > > powerpc64le cannot offload to itself.
> > > > > > > No, if the host has 128 b

[PATCH] D64289: [OpenMP] Fix 128-bit long double support on target

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:1594
+   !Context.getTargetInfo().hasFloat128Type() &&
+   Context.getTargetInfo().getLongDoubleWidth() != 128) ||
   (Ty->isIntegerType() && Context.getTypeSize(Ty) == 128 &&

jdenny wrote:
> ABataev wrote:
> > MaskRay wrote:
> > > ABataev wrote:
> > > > jdenny wrote:
> > > > > ABataev wrote:
> > > > > > jdenny wrote:
> > > > > > > ABataev wrote:
> > > > > > > > jdenny wrote:
> > > > > > > > > ABataev wrote:
> > > > > > > > > > jdenny wrote:
> > > > > > > > > > > ABataev wrote:
> > > > > > > > > > > > jdenny wrote:
> > > > > > > > > > > > > ABataev wrote:
> > > > > > > > > > > > > > Hmm, this look strange, at least. Seems to me, in 
> > > > > > > > > > > > > > this case the size of the long double is 128 bit 
> > > > > > > > > > > > > > (copied from the host), but device reports that it 
> > > > > > > > > > > > > > does not support 128 bit double. Seems to me, it is 
> > > > > > > > > > > > > > a problem with the device configuration. Why does 
> > > > > > > > > > > > > > the host translate long double to 128 bit fp, while 
> > > > > > > > > > > > > > the device translates it to 64 bit FP?
> > > > > > > > > > > > > Sorry, I think I've misunderstood what's happening 
> > > > > > > > > > > > > here, and my fix is probably wrong.
> > > > > > > > > > > > > 
> > > > > > > > > > > > > For x86_64, the example from my patch summary fails 
> > > > > > > > > > > > > as described there.  Does that work for you?
> > > > > > > > > > > > > 
> > > > > > > > > > > > > For powerpc64le, the reproducer I added to the test 
> > > > > > > > > > > > > suite fails without this patch.  Shouldn't it succeed?
> > > > > > > > > > > > Still, seems to me like the problem with the device 
> > > > > > > > > > > > config, not the original check.
> > > > > > > > > > > > Still, seems to me like the problem with the device 
> > > > > > > > > > > > config, not the original check.
> > > > > > > > > > > 
> > > > > > > > > > > I'm not sure where to begin looking for that.  Can you 
> > > > > > > > > > > point me in the right direction?  Thanks.
> > > > > > > > > > You need to understand why host and device report different 
> > > > > > > > > > size of the type. Check how the device is configured in 
> > > > > > > > > > lib/Basic/Targets
> > > > > > > > > Thanks for the pointer.  I think I understand things a bit 
> > > > > > > > > better now.
> > > > > > > > > 
> > > > > > > > > Without this patch's fix, the x86_64 example from this 
> > > > > > > > > patch's summary fails while this patch's new x86_64 test case 
> > > > > > > > > passes.  The difference is the summary's example doesn't 
> > > > > > > > > specify `-unknown-linux` after `x86_64`, and that's what sets 
> > > > > > > > > `hasFloat128Type()` to true.
> > > > > > > > > 
> > > > > > > > > `powerpc64le-unknown-linux-gnu` does not have `__float128`, 
> > > > > > > > > it seems.  That's why this patch's new powerpc64le test case 
> > > > > > > > > fails without this patch's fix.
> > > > > > > > > 
> > > > > > > > > It seems strange to me that the code we're commenting on 
> > > > > > > > > originally looks for the source type to be either 
> > > > > > > > > `__float128` or 128-bit `long double`, and it then requires 
> > > > > > > > > the target to support `__float128`.  It doesn't accept 
> > > > > > > > > 128-bit `long double` support as sufficient.  My intention in 
> > > > > > > > > this patch was to extend it to accept either so that all the 
> > > > > > > > > examples above compile.  Is that too lenient?  Am I 
> > > > > > > > > misinterpreting what's happening?
> > > > > > > > > 
> > > > > > > > > As for your comment about 64-bit floating point in the device 
> > > > > > > > > translation, I haven't seen that yet.  Did I miss it?
> > > > > > > > The intention of the original patch is to make host and device 
> > > > > > > > to have the same float128 and long double types. Device 
> > > > > > > > inherits those types from the host to be compatible during 
> > > > > > > > offloading and to correctly mangle functions.
> > > > > > > > Without this we just can't generate offloading regions 
> > > > > > > > correctly. If the host has 128 bit long double, the device also 
> > > > > > > > must have 128 bit long double. 
> > > > > > > > If device does not support 128bit floats, in this case device 
> > > > > > > > can only move the data (do load/stores ops only) and cannot do 
> > > > > > > > anything else.
> > > > > > > Are you intentionally requiring support for `__float128` when the 
> > > > > > > source type is 128-bit `long double`?  That seems to mean 
> > > > > > > powerpc64le cannot offload to itself.
> > > > > > No, if the host has 128 bit long double, the device must also have 
> > > > > > 128 bit long double. It has nothing to do with the float128 type 
> > > > > > itself.
> > > > > What if we change the logic to the following?
> > > > > 
> > > > > ```
> > > 

[PATCH] D64289: [OpenMP] Fix 128-bit long double support on target

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added inline comments.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:1594
+   !Context.getTargetInfo().hasFloat128Type() &&
+   Context.getTargetInfo().getLongDoubleWidth() != 128) ||
   (Ty->isIntegerType() && Context.getTypeSize(Ty) == 128 &&

ABataev wrote:
> MaskRay wrote:
> > ABataev wrote:
> > > jdenny wrote:
> > > > ABataev wrote:
> > > > > jdenny wrote:
> > > > > > ABataev wrote:
> > > > > > > jdenny wrote:
> > > > > > > > ABataev wrote:
> > > > > > > > > jdenny wrote:
> > > > > > > > > > ABataev wrote:
> > > > > > > > > > > jdenny wrote:
> > > > > > > > > > > > ABataev wrote:
> > > > > > > > > > > > > Hmm, this look strange, at least. Seems to me, in 
> > > > > > > > > > > > > this case the size of the long double is 128 bit 
> > > > > > > > > > > > > (copied from the host), but device reports that it 
> > > > > > > > > > > > > does not support 128 bit double. Seems to me, it is a 
> > > > > > > > > > > > > problem with the device configuration. Why does the 
> > > > > > > > > > > > > host translate long double to 128 bit fp, while the 
> > > > > > > > > > > > > device translates it to 64 bit FP?
> > > > > > > > > > > > Sorry, I think I've misunderstood what's happening 
> > > > > > > > > > > > here, and my fix is probably wrong.
> > > > > > > > > > > > 
> > > > > > > > > > > > For x86_64, the example from my patch summary fails as 
> > > > > > > > > > > > described there.  Does that work for you?
> > > > > > > > > > > > 
> > > > > > > > > > > > For powerpc64le, the reproducer I added to the test 
> > > > > > > > > > > > suite fails without this patch.  Shouldn't it succeed?
> > > > > > > > > > > Still, seems to me like the problem with the device 
> > > > > > > > > > > config, not the original check.
> > > > > > > > > > > Still, seems to me like the problem with the device 
> > > > > > > > > > > config, not the original check.
> > > > > > > > > > 
> > > > > > > > > > I'm not sure where to begin looking for that.  Can you 
> > > > > > > > > > point me in the right direction?  Thanks.
> > > > > > > > > You need to understand why host and device report different 
> > > > > > > > > size of the type. Check how the device is configured in 
> > > > > > > > > lib/Basic/Targets
> > > > > > > > Thanks for the pointer.  I think I understand things a bit 
> > > > > > > > better now.
> > > > > > > > 
> > > > > > > > Without this patch's fix, the x86_64 example from this patch's 
> > > > > > > > summary fails while this patch's new x86_64 test case passes.  
> > > > > > > > The difference is the summary's example doesn't specify 
> > > > > > > > `-unknown-linux` after `x86_64`, and that's what sets 
> > > > > > > > `hasFloat128Type()` to true.
> > > > > > > > 
> > > > > > > > `powerpc64le-unknown-linux-gnu` does not have `__float128`, it 
> > > > > > > > seems.  That's why this patch's new powerpc64le test case fails 
> > > > > > > > without this patch's fix.
> > > > > > > > 
> > > > > > > > It seems strange to me that the code we're commenting on 
> > > > > > > > originally looks for the source type to be either `__float128` 
> > > > > > > > or 128-bit `long double`, and it then requires the target to 
> > > > > > > > support `__float128`.  It doesn't accept 128-bit `long double` 
> > > > > > > > support as sufficient.  My intention in this patch was to 
> > > > > > > > extend it to accept either so that all the examples above 
> > > > > > > > compile.  Is that too lenient?  Am I misinterpreting what's 
> > > > > > > > happening?
> > > > > > > > 
> > > > > > > > As for your comment about 64-bit floating point in the device 
> > > > > > > > translation, I haven't seen that yet.  Did I miss it?
> > > > > > > The intention of the original patch is to make host and device to 
> > > > > > > have the same float128 and long double types. Device inherits 
> > > > > > > those types from the host to be compatible during offloading and 
> > > > > > > to correctly mangle functions.
> > > > > > > Without this we just can't generate offloading regions correctly. 
> > > > > > > If the host has 128 bit long double, the device also must have 
> > > > > > > 128 bit long double. 
> > > > > > > If device does not support 128bit floats, in this case device can 
> > > > > > > only move the data (do load/stores ops only) and cannot do 
> > > > > > > anything else.
> > > > > > Are you intentionally requiring support for `__float128` when the 
> > > > > > source type is 128-bit `long double`?  That seems to mean 
> > > > > > powerpc64le cannot offload to itself.
> > > > > No, if the host has 128 bit long double, the device must also have 
> > > > > 128 bit long double. It has nothing to do with the float128 type 
> > > > > itself.
> > > > What if we change the logic to the following?
> > > > 
> > > > ```
> > > > (Ty->isFloat128Type() && !Context.getTargetInfo().hasFloat128Type()) ||
> > > > (!Ty->isFloat128Type() && Ty->isRealFloatingType() &&
> > > >  Context.getTypeSize(Ty) == 128 &

[PATCH] D64289: [OpenMP] Fix 128-bit long double support on target

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:1594
+   !Context.getTargetInfo().hasFloat128Type() &&
+   Context.getTargetInfo().getLongDoubleWidth() != 128) ||
   (Ty->isIntegerType() && Context.getTypeSize(Ty) == 128 &&

MaskRay wrote:
> ABataev wrote:
> > jdenny wrote:
> > > ABataev wrote:
> > > > jdenny wrote:
> > > > > ABataev wrote:
> > > > > > jdenny wrote:
> > > > > > > ABataev wrote:
> > > > > > > > jdenny wrote:
> > > > > > > > > ABataev wrote:
> > > > > > > > > > jdenny wrote:
> > > > > > > > > > > ABataev wrote:
> > > > > > > > > > > > Hmm, this look strange, at least. Seems to me, in this 
> > > > > > > > > > > > case the size of the long double is 128 bit (copied 
> > > > > > > > > > > > from the host), but device reports that it does not 
> > > > > > > > > > > > support 128 bit double. Seems to me, it is a problem 
> > > > > > > > > > > > with the device configuration. Why does the host 
> > > > > > > > > > > > translate long double to 128 bit fp, while the device 
> > > > > > > > > > > > translates it to 64 bit FP?
> > > > > > > > > > > Sorry, I think I've misunderstood what's happening here, 
> > > > > > > > > > > and my fix is probably wrong.
> > > > > > > > > > > 
> > > > > > > > > > > For x86_64, the example from my patch summary fails as 
> > > > > > > > > > > described there.  Does that work for you?
> > > > > > > > > > > 
> > > > > > > > > > > For powerpc64le, the reproducer I added to the test suite 
> > > > > > > > > > > fails without this patch.  Shouldn't it succeed?
> > > > > > > > > > Still, seems to me like the problem with the device config, 
> > > > > > > > > > not the original check.
> > > > > > > > > > Still, seems to me like the problem with the device config, 
> > > > > > > > > > not the original check.
> > > > > > > > > 
> > > > > > > > > I'm not sure where to begin looking for that.  Can you point 
> > > > > > > > > me in the right direction?  Thanks.
> > > > > > > > You need to understand why host and device report different 
> > > > > > > > size of the type. Check how the device is configured in 
> > > > > > > > lib/Basic/Targets
> > > > > > > Thanks for the pointer.  I think I understand things a bit better 
> > > > > > > now.
> > > > > > > 
> > > > > > > Without this patch's fix, the x86_64 example from this patch's 
> > > > > > > summary fails while this patch's new x86_64 test case passes.  
> > > > > > > The difference is the summary's example doesn't specify 
> > > > > > > `-unknown-linux` after `x86_64`, and that's what sets 
> > > > > > > `hasFloat128Type()` to true.
> > > > > > > 
> > > > > > > `powerpc64le-unknown-linux-gnu` does not have `__float128`, it 
> > > > > > > seems.  That's why this patch's new powerpc64le test case fails 
> > > > > > > without this patch's fix.
> > > > > > > 
> > > > > > > It seems strange to me that the code we're commenting on 
> > > > > > > originally looks for the source type to be either `__float128` or 
> > > > > > > 128-bit `long double`, and it then requires the target to support 
> > > > > > > `__float128`.  It doesn't accept 128-bit `long double` support as 
> > > > > > > sufficient.  My intention in this patch was to extend it to 
> > > > > > > accept either so that all the examples above compile.  Is that 
> > > > > > > too lenient?  Am I misinterpreting what's happening?
> > > > > > > 
> > > > > > > As for your comment about 64-bit floating point in the device 
> > > > > > > translation, I haven't seen that yet.  Did I miss it?
> > > > > > The intention of the original patch is to make host and device to 
> > > > > > have the same float128 and long double types. Device inherits those 
> > > > > > types from the host to be compatible during offloading and to 
> > > > > > correctly mangle functions.
> > > > > > Without this we just can't generate offloading regions correctly. 
> > > > > > If the host has 128 bit long double, the device also must have 128 
> > > > > > bit long double. 
> > > > > > If device does not support 128bit floats, in this case device can 
> > > > > > only move the data (do load/stores ops only) and cannot do anything 
> > > > > > else.
> > > > > Are you intentionally requiring support for `__float128` when the 
> > > > > source type is 128-bit `long double`?  That seems to mean powerpc64le 
> > > > > cannot offload to itself.
> > > > No, if the host has 128 bit long double, the device must also have 128 
> > > > bit long double. It has nothing to do with the float128 type itself.
> > > What if we change the logic to the following?
> > > 
> > > ```
> > > (Ty->isFloat128Type() && !Context.getTargetInfo().hasFloat128Type()) ||
> > > (!Ty->isFloat128Type() && Ty->isRealFloatingType() &&
> > >  Context.getTypeSize(Ty) == 128 &&
> > >  Context.getTargetInfo().getLongDoubleWidth() != 128) 
> > > ```
> > > 
> > > Maybe there's a more succinct way to check if `Ty` is `long double`
> > What if `Ty` is not long double, but some 

[PATCH] D64289: [OpenMP] Fix 128-bit long double support on target

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added inline comments.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:1594
+   !Context.getTargetInfo().hasFloat128Type() &&
+   Context.getTargetInfo().getLongDoubleWidth() != 128) ||
   (Ty->isIntegerType() && Context.getTypeSize(Ty) == 128 &&

ABataev wrote:
> jdenny wrote:
> > ABataev wrote:
> > > jdenny wrote:
> > > > ABataev wrote:
> > > > > jdenny wrote:
> > > > > > ABataev wrote:
> > > > > > > jdenny wrote:
> > > > > > > > ABataev wrote:
> > > > > > > > > jdenny wrote:
> > > > > > > > > > ABataev wrote:
> > > > > > > > > > > Hmm, this look strange, at least. Seems to me, in this 
> > > > > > > > > > > case the size of the long double is 128 bit (copied from 
> > > > > > > > > > > the host), but device reports that it does not support 
> > > > > > > > > > > 128 bit double. Seems to me, it is a problem with the 
> > > > > > > > > > > device configuration. Why does the host translate long 
> > > > > > > > > > > double to 128 bit fp, while the device translates it to 
> > > > > > > > > > > 64 bit FP?
> > > > > > > > > > Sorry, I think I've misunderstood what's happening here, 
> > > > > > > > > > and my fix is probably wrong.
> > > > > > > > > > 
> > > > > > > > > > For x86_64, the example from my patch summary fails as 
> > > > > > > > > > described there.  Does that work for you?
> > > > > > > > > > 
> > > > > > > > > > For powerpc64le, the reproducer I added to the test suite 
> > > > > > > > > > fails without this patch.  Shouldn't it succeed?
> > > > > > > > > Still, seems to me like the problem with the device config, 
> > > > > > > > > not the original check.
> > > > > > > > > Still, seems to me like the problem with the device config, 
> > > > > > > > > not the original check.
> > > > > > > > 
> > > > > > > > I'm not sure where to begin looking for that.  Can you point me 
> > > > > > > > in the right direction?  Thanks.
> > > > > > > You need to understand why host and device report different size 
> > > > > > > of the type. Check how the device is configured in 
> > > > > > > lib/Basic/Targets
> > > > > > Thanks for the pointer.  I think I understand things a bit better 
> > > > > > now.
> > > > > > 
> > > > > > Without this patch's fix, the x86_64 example from this patch's 
> > > > > > summary fails while this patch's new x86_64 test case passes.  The 
> > > > > > difference is the summary's example doesn't specify 
> > > > > > `-unknown-linux` after `x86_64`, and that's what sets 
> > > > > > `hasFloat128Type()` to true.
> > > > > > 
> > > > > > `powerpc64le-unknown-linux-gnu` does not have `__float128`, it 
> > > > > > seems.  That's why this patch's new powerpc64le test case fails 
> > > > > > without this patch's fix.
> > > > > > 
> > > > > > It seems strange to me that the code we're commenting on originally 
> > > > > > looks for the source type to be either `__float128` or 128-bit 
> > > > > > `long double`, and it then requires the target to support 
> > > > > > `__float128`.  It doesn't accept 128-bit `long double` support as 
> > > > > > sufficient.  My intention in this patch was to extend it to accept 
> > > > > > either so that all the examples above compile.  Is that too 
> > > > > > lenient?  Am I misinterpreting what's happening?
> > > > > > 
> > > > > > As for your comment about 64-bit floating point in the device 
> > > > > > translation, I haven't seen that yet.  Did I miss it?
> > > > > The intention of the original patch is to make host and device to 
> > > > > have the same float128 and long double types. Device inherits those 
> > > > > types from the host to be compatible during offloading and to 
> > > > > correctly mangle functions.
> > > > > Without this we just can't generate offloading regions correctly. If 
> > > > > the host has 128 bit long double, the device also must have 128 bit 
> > > > > long double. 
> > > > > If device does not support 128bit floats, in this case device can 
> > > > > only move the data (do load/stores ops only) and cannot do anything 
> > > > > else.
> > > > Are you intentionally requiring support for `__float128` when the 
> > > > source type is 128-bit `long double`?  That seems to mean powerpc64le 
> > > > cannot offload to itself.
> > > No, if the host has 128 bit long double, the device must also have 128 
> > > bit long double. It has nothing to do with the float128 type itself.
> > What if we change the logic to the following?
> > 
> > ```
> > (Ty->isFloat128Type() && !Context.getTargetInfo().hasFloat128Type()) ||
> > (!Ty->isFloat128Type() && Ty->isRealFloatingType() &&
> >  Context.getTypeSize(Ty) == 128 &&
> >  Context.getTargetInfo().getLongDoubleWidth() != 128) 
> > ```
> > 
> > Maybe there's a more succinct way to check if `Ty` is `long double`
> What if `Ty` is not long double, but some other FP type?
I know little about OpenMP... but does these lines take into account of 128-bit 
IBM extended double on powerpc{32,64}? It is the default representation of 
`long double`

[PATCH] D64289: [OpenMP] Fix 128-bit long double support on target

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

Drive-by comment :)

In gcc, (1) -mlong-double-128 uses IEEE 754 quad as the representation of long 
double on x86_64 (2) -mlong-double-128 -mabi=ieeelongdouble uses IEEE 754 quad 
as the representation of long double on powerpc{32,64}.   (On Linux 
powerpc{32,64}, `-mlong-double-128 -mabi=ibmlongdouble` is the default, it uses 
a IBM 128-bit extended precision for long double).

`-mlong-double-128` is currently not supported by clang, but I intend to 
support it in D64277 . After that, the 
representation of `long double` will be the same as `__float128` (unfortunately 
they will use the same mangling scheme)

As I noticed, the mangling scheme of `__float128` also needs a fix. After 
applying D64277  and its dependent revisions, 
the last two CHECK lines of test/OpenMP/nvptx_unsupported_type_codegen.cpp will 
fail...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64289



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

[PATCH] D64289: [OpenMP] Fix 128-bit long double support on target

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:1594
+   !Context.getTargetInfo().hasFloat128Type() &&
+   Context.getTargetInfo().getLongDoubleWidth() != 128) ||
   (Ty->isIntegerType() && Context.getTypeSize(Ty) == 128 &&

jdenny wrote:
> ABataev wrote:
> > jdenny wrote:
> > > ABataev wrote:
> > > > jdenny wrote:
> > > > > ABataev wrote:
> > > > > > jdenny wrote:
> > > > > > > ABataev wrote:
> > > > > > > > jdenny wrote:
> > > > > > > > > ABataev wrote:
> > > > > > > > > > Hmm, this look strange, at least. Seems to me, in this case 
> > > > > > > > > > the size of the long double is 128 bit (copied from the 
> > > > > > > > > > host), but device reports that it does not support 128 bit 
> > > > > > > > > > double. Seems to me, it is a problem with the device 
> > > > > > > > > > configuration. Why does the host translate long double to 
> > > > > > > > > > 128 bit fp, while the device translates it to 64 bit FP?
> > > > > > > > > Sorry, I think I've misunderstood what's happening here, and 
> > > > > > > > > my fix is probably wrong.
> > > > > > > > > 
> > > > > > > > > For x86_64, the example from my patch summary fails as 
> > > > > > > > > described there.  Does that work for you?
> > > > > > > > > 
> > > > > > > > > For powerpc64le, the reproducer I added to the test suite 
> > > > > > > > > fails without this patch.  Shouldn't it succeed?
> > > > > > > > Still, seems to me like the problem with the device config, not 
> > > > > > > > the original check.
> > > > > > > > Still, seems to me like the problem with the device config, not 
> > > > > > > > the original check.
> > > > > > > 
> > > > > > > I'm not sure where to begin looking for that.  Can you point me 
> > > > > > > in the right direction?  Thanks.
> > > > > > You need to understand why host and device report different size of 
> > > > > > the type. Check how the device is configured in lib/Basic/Targets
> > > > > Thanks for the pointer.  I think I understand things a bit better now.
> > > > > 
> > > > > Without this patch's fix, the x86_64 example from this patch's 
> > > > > summary fails while this patch's new x86_64 test case passes.  The 
> > > > > difference is the summary's example doesn't specify `-unknown-linux` 
> > > > > after `x86_64`, and that's what sets `hasFloat128Type()` to true.
> > > > > 
> > > > > `powerpc64le-unknown-linux-gnu` does not have `__float128`, it seems. 
> > > > >  That's why this patch's new powerpc64le test case fails without this 
> > > > > patch's fix.
> > > > > 
> > > > > It seems strange to me that the code we're commenting on originally 
> > > > > looks for the source type to be either `__float128` or 128-bit `long 
> > > > > double`, and it then requires the target to support `__float128`.  It 
> > > > > doesn't accept 128-bit `long double` support as sufficient.  My 
> > > > > intention in this patch was to extend it to accept either so that all 
> > > > > the examples above compile.  Is that too lenient?  Am I 
> > > > > misinterpreting what's happening?
> > > > > 
> > > > > As for your comment about 64-bit floating point in the device 
> > > > > translation, I haven't seen that yet.  Did I miss it?
> > > > The intention of the original patch is to make host and device to have 
> > > > the same float128 and long double types. Device inherits those types 
> > > > from the host to be compatible during offloading and to correctly 
> > > > mangle functions.
> > > > Without this we just can't generate offloading regions correctly. If 
> > > > the host has 128 bit long double, the device also must have 128 bit 
> > > > long double. 
> > > > If device does not support 128bit floats, in this case device can only 
> > > > move the data (do load/stores ops only) and cannot do anything else.
> > > Are you intentionally requiring support for `__float128` when the source 
> > > type is 128-bit `long double`?  That seems to mean powerpc64le cannot 
> > > offload to itself.
> > No, if the host has 128 bit long double, the device must also have 128 bit 
> > long double. It has nothing to do with the float128 type itself.
> What if we change the logic to the following?
> 
> ```
> (Ty->isFloat128Type() && !Context.getTargetInfo().hasFloat128Type()) ||
> (!Ty->isFloat128Type() && Ty->isRealFloatingType() &&
>  Context.getTypeSize(Ty) == 128 &&
>  Context.getTargetInfo().getLongDoubleWidth() != 128) 
> ```
> 
> Maybe there's a more succinct way to check if `Ty` is `long double`
What if `Ty` is not long double, but some other FP type?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64289



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

[PATCH] D64289: [OpenMP] Fix 128-bit long double support on target

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added inline comments.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:1594
+   !Context.getTargetInfo().hasFloat128Type() &&
+   Context.getTargetInfo().getLongDoubleWidth() != 128) ||
   (Ty->isIntegerType() && Context.getTypeSize(Ty) == 128 &&

ABataev wrote:
> jdenny wrote:
> > ABataev wrote:
> > > jdenny wrote:
> > > > ABataev wrote:
> > > > > jdenny wrote:
> > > > > > ABataev wrote:
> > > > > > > jdenny wrote:
> > > > > > > > ABataev wrote:
> > > > > > > > > Hmm, this look strange, at least. Seems to me, in this case 
> > > > > > > > > the size of the long double is 128 bit (copied from the 
> > > > > > > > > host), but device reports that it does not support 128 bit 
> > > > > > > > > double. Seems to me, it is a problem with the device 
> > > > > > > > > configuration. Why does the host translate long double to 128 
> > > > > > > > > bit fp, while the device translates it to 64 bit FP?
> > > > > > > > Sorry, I think I've misunderstood what's happening here, and my 
> > > > > > > > fix is probably wrong.
> > > > > > > > 
> > > > > > > > For x86_64, the example from my patch summary fails as 
> > > > > > > > described there.  Does that work for you?
> > > > > > > > 
> > > > > > > > For powerpc64le, the reproducer I added to the test suite fails 
> > > > > > > > without this patch.  Shouldn't it succeed?
> > > > > > > Still, seems to me like the problem with the device config, not 
> > > > > > > the original check.
> > > > > > > Still, seems to me like the problem with the device config, not 
> > > > > > > the original check.
> > > > > > 
> > > > > > I'm not sure where to begin looking for that.  Can you point me in 
> > > > > > the right direction?  Thanks.
> > > > > You need to understand why host and device report different size of 
> > > > > the type. Check how the device is configured in lib/Basic/Targets
> > > > Thanks for the pointer.  I think I understand things a bit better now.
> > > > 
> > > > Without this patch's fix, the x86_64 example from this patch's summary 
> > > > fails while this patch's new x86_64 test case passes.  The difference 
> > > > is the summary's example doesn't specify `-unknown-linux` after 
> > > > `x86_64`, and that's what sets `hasFloat128Type()` to true.
> > > > 
> > > > `powerpc64le-unknown-linux-gnu` does not have `__float128`, it seems.  
> > > > That's why this patch's new powerpc64le test case fails without this 
> > > > patch's fix.
> > > > 
> > > > It seems strange to me that the code we're commenting on originally 
> > > > looks for the source type to be either `__float128` or 128-bit `long 
> > > > double`, and it then requires the target to support `__float128`.  It 
> > > > doesn't accept 128-bit `long double` support as sufficient.  My 
> > > > intention in this patch was to extend it to accept either so that all 
> > > > the examples above compile.  Is that too lenient?  Am I misinterpreting 
> > > > what's happening?
> > > > 
> > > > As for your comment about 64-bit floating point in the device 
> > > > translation, I haven't seen that yet.  Did I miss it?
> > > The intention of the original patch is to make host and device to have 
> > > the same float128 and long double types. Device inherits those types from 
> > > the host to be compatible during offloading and to correctly mangle 
> > > functions.
> > > Without this we just can't generate offloading regions correctly. If the 
> > > host has 128 bit long double, the device also must have 128 bit long 
> > > double. 
> > > If device does not support 128bit floats, in this case device can only 
> > > move the data (do load/stores ops only) and cannot do anything else.
> > Are you intentionally requiring support for `__float128` when the source 
> > type is 128-bit `long double`?  That seems to mean powerpc64le cannot 
> > offload to itself.
> No, if the host has 128 bit long double, the device must also have 128 bit 
> long double. It has nothing to do with the float128 type itself.
What if we change the logic to the following?

```
(Ty->isFloat128Type() && !Context.getTargetInfo().hasFloat128Type()) ||
(!Ty->isFloat128Type() && Ty->isRealFloatingType() &&
 Context.getTypeSize(Ty) == 128 &&
 Context.getTargetInfo().getLongDoubleWidth() != 128) 
```

Maybe there's a more succinct way to check if `Ty` is `long double`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64289



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

[PATCH] D64289: [OpenMP] Fix 128-bit long double support on target

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:1594
+   !Context.getTargetInfo().hasFloat128Type() &&
+   Context.getTargetInfo().getLongDoubleWidth() != 128) ||
   (Ty->isIntegerType() && Context.getTypeSize(Ty) == 128 &&

jdenny wrote:
> ABataev wrote:
> > jdenny wrote:
> > > ABataev wrote:
> > > > jdenny wrote:
> > > > > ABataev wrote:
> > > > > > jdenny wrote:
> > > > > > > ABataev wrote:
> > > > > > > > Hmm, this look strange, at least. Seems to me, in this case the 
> > > > > > > > size of the long double is 128 bit (copied from the host), but 
> > > > > > > > device reports that it does not support 128 bit double. Seems 
> > > > > > > > to me, it is a problem with the device configuration. Why does 
> > > > > > > > the host translate long double to 128 bit fp, while the device 
> > > > > > > > translates it to 64 bit FP?
> > > > > > > Sorry, I think I've misunderstood what's happening here, and my 
> > > > > > > fix is probably wrong.
> > > > > > > 
> > > > > > > For x86_64, the example from my patch summary fails as described 
> > > > > > > there.  Does that work for you?
> > > > > > > 
> > > > > > > For powerpc64le, the reproducer I added to the test suite fails 
> > > > > > > without this patch.  Shouldn't it succeed?
> > > > > > Still, seems to me like the problem with the device config, not the 
> > > > > > original check.
> > > > > > Still, seems to me like the problem with the device config, not the 
> > > > > > original check.
> > > > > 
> > > > > I'm not sure where to begin looking for that.  Can you point me in 
> > > > > the right direction?  Thanks.
> > > > You need to understand why host and device report different size of the 
> > > > type. Check how the device is configured in lib/Basic/Targets
> > > Thanks for the pointer.  I think I understand things a bit better now.
> > > 
> > > Without this patch's fix, the x86_64 example from this patch's summary 
> > > fails while this patch's new x86_64 test case passes.  The difference is 
> > > the summary's example doesn't specify `-unknown-linux` after `x86_64`, 
> > > and that's what sets `hasFloat128Type()` to true.
> > > 
> > > `powerpc64le-unknown-linux-gnu` does not have `__float128`, it seems.  
> > > That's why this patch's new powerpc64le test case fails without this 
> > > patch's fix.
> > > 
> > > It seems strange to me that the code we're commenting on originally looks 
> > > for the source type to be either `__float128` or 128-bit `long double`, 
> > > and it then requires the target to support `__float128`.  It doesn't 
> > > accept 128-bit `long double` support as sufficient.  My intention in this 
> > > patch was to extend it to accept either so that all the examples above 
> > > compile.  Is that too lenient?  Am I misinterpreting what's happening?
> > > 
> > > As for your comment about 64-bit floating point in the device 
> > > translation, I haven't seen that yet.  Did I miss it?
> > The intention of the original patch is to make host and device to have the 
> > same float128 and long double types. Device inherits those types from the 
> > host to be compatible during offloading and to correctly mangle functions.
> > Without this we just can't generate offloading regions correctly. If the 
> > host has 128 bit long double, the device also must have 128 bit long 
> > double. 
> > If device does not support 128bit floats, in this case device can only move 
> > the data (do load/stores ops only) and cannot do anything else.
> Are you intentionally requiring support for `__float128` when the source type 
> is 128-bit `long double`?  That seems to mean powerpc64le cannot offload to 
> itself.
No, if the host has 128 bit long double, the device must also have 128 bit long 
double. It has nothing to do with the float128 type itself.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64289



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

[PATCH] D64128: [CodeGen] Generate llvm.ptrmask instead of inttoptr(and(ptrtoint, C)) if possible.

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

I would be opposed to any addition of a `-f` flag for this absent any evidence 
that it's valuable for some actual C code; this patch appears to be purely 
speculative.  I certainly don't think we should be adding it default-on.  Your 
argument about alignment is what I was referring to when I mentioned that this 
is enforcing alignment restrictions in places we traditionally and 
intentionally haven't.

Note that it won't help Clang's major uses of `PointerIntPair` and 
`PointerUnion` because our traits say that types like `Decl` and `Type` have 
more alignment bits than their formal alignment would indicate, and 
`PointerIntPair` occupies alignment bits starting with the most significant.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64128



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

[PATCH] D64289: [OpenMP] Fix 128-bit long double support on target

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added inline comments.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:1594
+   !Context.getTargetInfo().hasFloat128Type() &&
+   Context.getTargetInfo().getLongDoubleWidth() != 128) ||
   (Ty->isIntegerType() && Context.getTypeSize(Ty) == 128 &&

ABataev wrote:
> jdenny wrote:
> > ABataev wrote:
> > > jdenny wrote:
> > > > ABataev wrote:
> > > > > jdenny wrote:
> > > > > > ABataev wrote:
> > > > > > > Hmm, this look strange, at least. Seems to me, in this case the 
> > > > > > > size of the long double is 128 bit (copied from the host), but 
> > > > > > > device reports that it does not support 128 bit double. Seems to 
> > > > > > > me, it is a problem with the device configuration. Why does the 
> > > > > > > host translate long double to 128 bit fp, while the device 
> > > > > > > translates it to 64 bit FP?
> > > > > > Sorry, I think I've misunderstood what's happening here, and my fix 
> > > > > > is probably wrong.
> > > > > > 
> > > > > > For x86_64, the example from my patch summary fails as described 
> > > > > > there.  Does that work for you?
> > > > > > 
> > > > > > For powerpc64le, the reproducer I added to the test suite fails 
> > > > > > without this patch.  Shouldn't it succeed?
> > > > > Still, seems to me like the problem with the device config, not the 
> > > > > original check.
> > > > > Still, seems to me like the problem with the device config, not the 
> > > > > original check.
> > > > 
> > > > I'm not sure where to begin looking for that.  Can you point me in the 
> > > > right direction?  Thanks.
> > > You need to understand why host and device report different size of the 
> > > type. Check how the device is configured in lib/Basic/Targets
> > Thanks for the pointer.  I think I understand things a bit better now.
> > 
> > Without this patch's fix, the x86_64 example from this patch's summary 
> > fails while this patch's new x86_64 test case passes.  The difference is 
> > the summary's example doesn't specify `-unknown-linux` after `x86_64`, and 
> > that's what sets `hasFloat128Type()` to true.
> > 
> > `powerpc64le-unknown-linux-gnu` does not have `__float128`, it seems.  
> > That's why this patch's new powerpc64le test case fails without this 
> > patch's fix.
> > 
> > It seems strange to me that the code we're commenting on originally looks 
> > for the source type to be either `__float128` or 128-bit `long double`, and 
> > it then requires the target to support `__float128`.  It doesn't accept 
> > 128-bit `long double` support as sufficient.  My intention in this patch 
> > was to extend it to accept either so that all the examples above compile.  
> > Is that too lenient?  Am I misinterpreting what's happening?
> > 
> > As for your comment about 64-bit floating point in the device translation, 
> > I haven't seen that yet.  Did I miss it?
> The intention of the original patch is to make host and device to have the 
> same float128 and long double types. Device inherits those types from the 
> host to be compatible during offloading and to correctly mangle functions.
> Without this we just can't generate offloading regions correctly. If the host 
> has 128 bit long double, the device also must have 128 bit long double. 
> If device does not support 128bit floats, in this case device can only move 
> the data (do load/stores ops only) and cannot do anything else.
Are you intentionally requiring support for `__float128` when the source type 
is 128-bit `long double`?  That seems to mean powerpc64le cannot offload to 
itself.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64289



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

[PATCH] D64289: [OpenMP] Fix 128-bit long double support on target

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:1594
+   !Context.getTargetInfo().hasFloat128Type() &&
+   Context.getTargetInfo().getLongDoubleWidth() != 128) ||
   (Ty->isIntegerType() && Context.getTypeSize(Ty) == 128 &&

jdenny wrote:
> ABataev wrote:
> > jdenny wrote:
> > > ABataev wrote:
> > > > jdenny wrote:
> > > > > ABataev wrote:
> > > > > > Hmm, this look strange, at least. Seems to me, in this case the 
> > > > > > size of the long double is 128 bit (copied from the host), but 
> > > > > > device reports that it does not support 128 bit double. Seems to 
> > > > > > me, it is a problem with the device configuration. Why does the 
> > > > > > host translate long double to 128 bit fp, while the device 
> > > > > > translates it to 64 bit FP?
> > > > > Sorry, I think I've misunderstood what's happening here, and my fix 
> > > > > is probably wrong.
> > > > > 
> > > > > For x86_64, the example from my patch summary fails as described 
> > > > > there.  Does that work for you?
> > > > > 
> > > > > For powerpc64le, the reproducer I added to the test suite fails 
> > > > > without this patch.  Shouldn't it succeed?
> > > > Still, seems to me like the problem with the device config, not the 
> > > > original check.
> > > > Still, seems to me like the problem with the device config, not the 
> > > > original check.
> > > 
> > > I'm not sure where to begin looking for that.  Can you point me in the 
> > > right direction?  Thanks.
> > You need to understand why host and device report different size of the 
> > type. Check how the device is configured in lib/Basic/Targets
> Thanks for the pointer.  I think I understand things a bit better now.
> 
> Without this patch's fix, the x86_64 example from this patch's summary fails 
> while this patch's new x86_64 test case passes.  The difference is the 
> summary's example doesn't specify `-unknown-linux` after `x86_64`, and that's 
> what sets `hasFloat128Type()` to true.
> 
> `powerpc64le-unknown-linux-gnu` does not have `__float128`, it seems.  That's 
> why this patch's new powerpc64le test case fails without this patch's fix.
> 
> It seems strange to me that the code we're commenting on originally looks for 
> the source type to be either `__float128` or 128-bit `long double`, and it 
> then requires the target to support `__float128`.  It doesn't accept 128-bit 
> `long double` support as sufficient.  My intention in this patch was to 
> extend it to accept either so that all the examples above compile.  Is that 
> too lenient?  Am I misinterpreting what's happening?
> 
> As for your comment about 64-bit floating point in the device translation, I 
> haven't seen that yet.  Did I miss it?
The intention of the original patch is to make host and device to have the same 
float128 and long double types. Device inherits those types from the host to be 
compatible during offloading and to correctly mangle functions.
Without this we just can't generate offloading regions correctly. If the host 
has 128 bit long double, the device also must have 128 bit long double. 
If device does not support 128bit floats, in this case device can only move the 
data (do load/stores ops only) and cannot do anything else.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64289



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

[PATCH] D64289: [OpenMP] Fix 128-bit long double support on target

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added inline comments.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:1594
+   !Context.getTargetInfo().hasFloat128Type() &&
+   Context.getTargetInfo().getLongDoubleWidth() != 128) ||
   (Ty->isIntegerType() && Context.getTypeSize(Ty) == 128 &&

ABataev wrote:
> jdenny wrote:
> > ABataev wrote:
> > > jdenny wrote:
> > > > ABataev wrote:
> > > > > Hmm, this look strange, at least. Seems to me, in this case the size 
> > > > > of the long double is 128 bit (copied from the host), but device 
> > > > > reports that it does not support 128 bit double. Seems to me, it is a 
> > > > > problem with the device configuration. Why does the host translate 
> > > > > long double to 128 bit fp, while the device translates it to 64 bit 
> > > > > FP?
> > > > Sorry, I think I've misunderstood what's happening here, and my fix is 
> > > > probably wrong.
> > > > 
> > > > For x86_64, the example from my patch summary fails as described there. 
> > > >  Does that work for you?
> > > > 
> > > > For powerpc64le, the reproducer I added to the test suite fails without 
> > > > this patch.  Shouldn't it succeed?
> > > Still, seems to me like the problem with the device config, not the 
> > > original check.
> > > Still, seems to me like the problem with the device config, not the 
> > > original check.
> > 
> > I'm not sure where to begin looking for that.  Can you point me in the 
> > right direction?  Thanks.
> You need to understand why host and device report different size of the type. 
> Check how the device is configured in lib/Basic/Targets
Thanks for the pointer.  I think I understand things a bit better now.

Without this patch's fix, the x86_64 example from this patch's summary fails 
while this patch's new x86_64 test case passes.  The difference is the 
summary's example doesn't specify `-unknown-linux` after `x86_64`, and that's 
what sets `hasFloat128Type()` to true.

`powerpc64le-unknown-linux-gnu` does not have `__float128`, it seems.  That's 
why this patch's new powerpc64le test case fails without this patch's fix.

It seems strange to me that the code we're commenting on originally looks for 
the source type to be either `__float128` or 128-bit `long double`, and it then 
requires the target to support `__float128`.  It doesn't accept 128-bit `long 
double` support as sufficient.  My intention in this patch was to extend it to 
accept either so that all the examples above compile.  Is that too lenient?  Am 
I misinterpreting what's happening?

As for your comment about 64-bit floating point in the device translation, I 
haven't seen that yet.  Did I miss it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64289



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

[PATCH] D64289: [OpenMP] Fix 128-bit long double support on target

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:1594
+   !Context.getTargetInfo().hasFloat128Type() &&
+   Context.getTargetInfo().getLongDoubleWidth() != 128) ||
   (Ty->isIntegerType() && Context.getTypeSize(Ty) == 128 &&

jdenny wrote:
> ABataev wrote:
> > jdenny wrote:
> > > ABataev wrote:
> > > > Hmm, this look strange, at least. Seems to me, in this case the size of 
> > > > the long double is 128 bit (copied from the host), but device reports 
> > > > that it does not support 128 bit double. Seems to me, it is a problem 
> > > > with the device configuration. Why does the host translate long double 
> > > > to 128 bit fp, while the device translates it to 64 bit FP?
> > > Sorry, I think I've misunderstood what's happening here, and my fix is 
> > > probably wrong.
> > > 
> > > For x86_64, the example from my patch summary fails as described there.  
> > > Does that work for you?
> > > 
> > > For powerpc64le, the reproducer I added to the test suite fails without 
> > > this patch.  Shouldn't it succeed?
> > Still, seems to me like the problem with the device config, not the 
> > original check.
> > Still, seems to me like the problem with the device config, not the 
> > original check.
> 
> I'm not sure where to begin looking for that.  Can you point me in the right 
> direction?  Thanks.
You need to understand why host and device report different size of the type. 
Check how the device is configured in lib/Basic/Targets


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64289



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

[PATCH] D64289: [OpenMP] Fix 128-bit long double support on target

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added inline comments.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:1594
+   !Context.getTargetInfo().hasFloat128Type() &&
+   Context.getTargetInfo().getLongDoubleWidth() != 128) ||
   (Ty->isIntegerType() && Context.getTypeSize(Ty) == 128 &&

ABataev wrote:
> jdenny wrote:
> > ABataev wrote:
> > > Hmm, this look strange, at least. Seems to me, in this case the size of 
> > > the long double is 128 bit (copied from the host), but device reports 
> > > that it does not support 128 bit double. Seems to me, it is a problem 
> > > with the device configuration. Why does the host translate long double to 
> > > 128 bit fp, while the device translates it to 64 bit FP?
> > Sorry, I think I've misunderstood what's happening here, and my fix is 
> > probably wrong.
> > 
> > For x86_64, the example from my patch summary fails as described there.  
> > Does that work for you?
> > 
> > For powerpc64le, the reproducer I added to the test suite fails without 
> > this patch.  Shouldn't it succeed?
> Still, seems to me like the problem with the device config, not the original 
> check.
> Still, seems to me like the problem with the device config, not the original 
> check.

I'm not sure where to begin looking for that.  Can you point me in the right 
direction?  Thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64289



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

[PATCH] D64289: [OpenMP] Fix 128-bit long double support on target

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:1594
+   !Context.getTargetInfo().hasFloat128Type() &&
+   Context.getTargetInfo().getLongDoubleWidth() != 128) ||
   (Ty->isIntegerType() && Context.getTypeSize(Ty) == 128 &&

jdenny wrote:
> ABataev wrote:
> > Hmm, this look strange, at least. Seems to me, in this case the size of the 
> > long double is 128 bit (copied from the host), but device reports that it 
> > does not support 128 bit double. Seems to me, it is a problem with the 
> > device configuration. Why does the host translate long double to 128 bit 
> > fp, while the device translates it to 64 bit FP?
> Sorry, I think I've misunderstood what's happening here, and my fix is 
> probably wrong.
> 
> For x86_64, the example from my patch summary fails as described there.  Does 
> that work for you?
> 
> For powerpc64le, the reproducer I added to the test suite fails without this 
> patch.  Shouldn't it succeed?
Still, seems to me like the problem with the device config, not the original 
check.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64289



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

[PATCH] D63793: Treat the range of representable values of floating-point types as [-inf, +inf] not as [-max, +max].

[Richard Smith - zygoloid via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL365272: Treat the range of representable values of 
floating-point types as [-inf, +inf]… (authored by rsmith, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D63793?vs=206532&id=208283#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D63793

Files:
  cfe/trunk/docs/UndefinedBehaviorSanitizer.rst
  cfe/trunk/include/clang/Basic/Sanitizers.def
  cfe/trunk/lib/AST/ExprConstant.cpp
  cfe/trunk/lib/CodeGen/CGExprScalar.cpp
  cfe/trunk/test/CXX/expr/expr.const/p2-0x.cpp
  cfe/trunk/test/CodeGen/catch-undef-behavior.c
  cfe/trunk/test/Driver/fsanitize.c
  cfe/trunk/test/SemaCXX/constant-expression-cxx1y.cpp

Index: cfe/trunk/lib/CodeGen/CGExprScalar.cpp
===
--- cfe/trunk/lib/CodeGen/CGExprScalar.cpp
+++ cfe/trunk/lib/CodeGen/CGExprScalar.cpp
@@ -313,7 +313,7 @@
   /// boolean (i1) truth value.  This is equivalent to "Val != 0".
   Value *EmitConversionToBool(Value *Src, QualType DstTy);
 
-  /// Emit a check that a conversion to or from a floating-point type does not
+  /// Emit a check that a conversion from a floating-point type does not
   /// overflow.
   void EmitFloatConversionCheck(Value *OrigSrc, QualType OrigSrcType,
 Value *Src, QualType SrcType, QualType DstType,
@@ -864,128 +864,63 @@
 void ScalarExprEmitter::EmitFloatConversionCheck(
 Value *OrigSrc, QualType OrigSrcType, Value *Src, QualType SrcType,
 QualType DstType, llvm::Type *DstTy, SourceLocation Loc) {
+  assert(SrcType->isFloatingType() && "not a conversion from floating point");
+  if (!isa(DstTy))
+return;
+
   CodeGenFunction::SanitizerScope SanScope(&CGF);
   using llvm::APFloat;
   using llvm::APSInt;
 
-  llvm::Type *SrcTy = Src->getType();
-
   llvm::Value *Check = nullptr;
-  if (llvm::IntegerType *IntTy = dyn_cast(SrcTy)) {
-// Integer to floating-point. This can fail for unsigned short -> __half
-// or unsigned __int128 -> float.
-assert(DstType->isFloatingType());
-bool SrcIsUnsigned = OrigSrcType->isUnsignedIntegerOrEnumerationType();
-
-APFloat LargestFloat =
-  APFloat::getLargest(CGF.getContext().getFloatTypeSemantics(DstType));
-APSInt LargestInt(IntTy->getBitWidth(), SrcIsUnsigned);
-
-bool IsExact;
-if (LargestFloat.convertToInteger(LargestInt, APFloat::rmTowardZero,
-  &IsExact) != APFloat::opOK)
-  // The range of representable values of this floating point type includes
-  // all values of this integer type. Don't need an overflow check.
-  return;
+  const llvm::fltSemantics &SrcSema =
+CGF.getContext().getFloatTypeSemantics(OrigSrcType);
 
-llvm::Value *Max = llvm::ConstantInt::get(VMContext, LargestInt);
-if (SrcIsUnsigned)
-  Check = Builder.CreateICmpULE(Src, Max);
-else {
-  llvm::Value *Min = llvm::ConstantInt::get(VMContext, -LargestInt);
-  llvm::Value *GE = Builder.CreateICmpSGE(Src, Min);
-  llvm::Value *LE = Builder.CreateICmpSLE(Src, Max);
-  Check = Builder.CreateAnd(GE, LE);
-}
-  } else {
-const llvm::fltSemantics &SrcSema =
-  CGF.getContext().getFloatTypeSemantics(OrigSrcType);
-if (isa(DstTy)) {
-  // Floating-point to integer. This has undefined behavior if the source is
-  // +-Inf, NaN, or doesn't fit into the destination type (after truncation
-  // to an integer).
-  unsigned Width = CGF.getContext().getIntWidth(DstType);
-  bool Unsigned = DstType->isUnsignedIntegerOrEnumerationType();
-
-  APSInt Min = APSInt::getMinValue(Width, Unsigned);
-  APFloat MinSrc(SrcSema, APFloat::uninitialized);
-  if (MinSrc.convertFromAPInt(Min, !Unsigned, APFloat::rmTowardZero) &
-  APFloat::opOverflow)
-// Don't need an overflow check for lower bound. Just check for
-// -Inf/NaN.
-MinSrc = APFloat::getInf(SrcSema, true);
-  else
-// Find the largest value which is too small to represent (before
-// truncation toward zero).
-MinSrc.subtract(APFloat(SrcSema, 1), APFloat::rmTowardNegative);
-
-  APSInt Max = APSInt::getMaxValue(Width, Unsigned);
-  APFloat MaxSrc(SrcSema, APFloat::uninitialized);
-  if (MaxSrc.convertFromAPInt(Max, !Unsigned, APFloat::rmTowardZero) &
-  APFloat::opOverflow)
-// Don't need an overflow check for upper bound. Just check for
-// +Inf/NaN.
-MaxSrc = APFloat::getInf(SrcSema, false);
-  else
-// Find the smallest value which is too large to represent (before
-// truncation toward zero).
-MaxSrc.add(APFloat(SrcSema, 1), APFloat::rmTowardPositive);
-
-  // If we're converting from __half, convert the range to float to match
-  // th

r365272 - Treat the range of representable values of floating-point types as [-inf, +inf] not as [-max, +max].

[Richard Smith via cfe-commits]

Author: rsmith
Date: Sat Jul  6 14:05:52 2019
New Revision: 365272

URL: http://llvm.org/viewvc/llvm-project?rev=365272&view=rev
Log:
Treat the range of representable values of floating-point types as [-inf, +inf] 
not as [-max, +max].

Summary:
Prior to r329065, we used [-max, max] as the range of representable
values because LLVM's `fptrunc` did not guarantee defined behavior when
truncating from a larger floating-point type to a smaller one. Now that
has been fixed, we can make clang follow normal IEEE 754 semantics in this
regard and take the larger range [-inf, +inf] as the range of representable
values.

In practice, this affects two parts of the frontend:
 * the constant evaluator no longer treats floating-point evaluations
   that result in +-inf as being undefined (because they no longer leave
   the range of representable values of the type)
 * UBSan no longer treats conversions to floating-point type that are
   outside the [-max, +max] range as being undefined

In passing, also remove the float-divide-by-zero sanitizer from
-fsanitize=undefined, on the basis that while it's undefined per C++
rules (and we disallow it in constant expressions for that reason), it
is defined by Clang / LLVM / IEEE 754.

Reviewers: rnk, BillyONeal

Subscribers: cfe-commits

Tags: #clang

Differential Revision: https://reviews.llvm.org/D63793

Modified:
cfe/trunk/docs/UndefinedBehaviorSanitizer.rst
cfe/trunk/include/clang/Basic/Sanitizers.def
cfe/trunk/lib/AST/ExprConstant.cpp
cfe/trunk/lib/CodeGen/CGExprScalar.cpp
cfe/trunk/test/CXX/expr/expr.const/p2-0x.cpp
cfe/trunk/test/CodeGen/catch-undef-behavior.c
cfe/trunk/test/Driver/fsanitize.c
cfe/trunk/test/SemaCXX/constant-expression-cxx1y.cpp

Modified: cfe/trunk/docs/UndefinedBehaviorSanitizer.rst
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/docs/UndefinedBehaviorSanitizer.rst?rev=365272&r1=365271&r2=365272&view=diff
==
--- cfe/trunk/docs/UndefinedBehaviorSanitizer.rst (original)
+++ cfe/trunk/docs/UndefinedBehaviorSanitizer.rst Sat Jul  6 14:05:52 2019
@@ -83,9 +83,13 @@ Available checks are:
  type.
   -  ``-fsanitize=float-cast-overflow``: Conversion to, from, or
  between floating-point types which would overflow the
- destination.
+ destination. Because the range of representable values for all
+ floating-point types supported by Clang is [-inf, +inf], the only
+ cases detected are conversions from floating point to integer types.
   -  ``-fsanitize=float-divide-by-zero``: Floating point division by
- zero.
+ zero. This is undefined per the C and C++ standards, but is defined
+ by Clang (and by ISO/IEC/IEEE 60559 / IEEE 754) as producing either an
+ infinity or NaN value, so is not included in ``-fsanitize=undefined``.
   -  ``-fsanitize=function``: Indirect call of a function through a
  function pointer of the wrong type (Darwin/Linux, C++ and x86/x86_64
  only).
@@ -163,8 +167,8 @@ Available checks are:
 
 You can also use the following check groups:
   -  ``-fsanitize=undefined``: All of the checks listed above other than
- ``unsigned-integer-overflow``, ``implicit-conversion`` and the
- ``nullability-*`` group of checks.
+ ``float-divide-by-zero``, ``unsigned-integer-overflow``,
+ ``implicit-conversion``, and the ``nullability-*`` group of checks.
   -  ``-fsanitize=undefined-trap``: Deprecated alias of
  ``-fsanitize=undefined``.
   -  ``-fsanitize=implicit-integer-truncation``: Catches lossy integral
@@ -174,16 +178,16 @@ You can also use the following check gro
  conversions that change the arithmetic value of the integer. Enables
  ``implicit-signed-integer-truncation`` and 
``implicit-integer-sign-change``.
   -  ``-fsanitize=implicit-conversion``: Checks for suspicious
- behaviour of implicit conversions. Enables
+ behavior of implicit conversions. Enables
  ``implicit-unsigned-integer-truncation``,
- ``implicit-signed-integer-truncation`` and
+ ``implicit-signed-integer-truncation``, and
  ``implicit-integer-sign-change``.
   -  ``-fsanitize=integer``: Checks for undefined or suspicious integer
  behavior (e.g. unsigned integer overflow).
  Enables ``signed-integer-overflow``, ``unsigned-integer-overflow``,
  ``shift``, ``integer-divide-by-zero``,
  ``implicit-unsigned-integer-truncation``,
- ``implicit-signed-integer-truncation`` and
+ ``implicit-signed-integer-truncation``, and
  ``implicit-integer-sign-change``.
   -  ``-fsanitize=nullability``: Enables ``nullability-arg``,
  ``nullability-assign``, and ``nullability-return``. While violating

Modified: cfe/trunk/include/clang/Basic/Sanitizers.def
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/Sanitizers.def?rev=365272&r1=365271&r2=365272&view=diff
==
--- c

[PATCH] D64289: [OpenMP] Fix 128-bit long double support on target

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added inline comments.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:1594
+   !Context.getTargetInfo().hasFloat128Type() &&
+   Context.getTargetInfo().getLongDoubleWidth() != 128) ||
   (Ty->isIntegerType() && Context.getTypeSize(Ty) == 128 &&

ABataev wrote:
> Hmm, this look strange, at least. Seems to me, in this case the size of the 
> long double is 128 bit (copied from the host), but device reports that it 
> does not support 128 bit double. Seems to me, it is a problem with the device 
> configuration. Why does the host translate long double to 128 bit fp, while 
> the device translates it to 64 bit FP?
Sorry, I think I've misunderstood what's happening here, and my fix is probably 
wrong.

For x86_64, the example from my patch summary fails as described there.  Does 
that work for you?

For powerpc64le, the reproducer I added to the test suite fails without this 
patch.  Shouldn't it succeed?



Comment at: clang/test/OpenMP/nvptx_unsupported_type_messages.cpp:9
+// RUN: %clang_cc1 -verify=quiet -fopenmp -x c++ -triple x86_64-unknown-linux 
-fopenmp-targets=x86_64-unknown-linux -emit-llvm-bc %s -o %t-host.bc
+// RUN: %clang_cc1 -verify=quiet -fopenmp -x c++ -triple 
powerpc64le-unknown-linux-gnu -aux-triple x86_64-unknown-linux 
-fopenmp-targets=x86_64-unknown-linux %s -fopenmp-is-device 
-fopenmp-host-ir-file-path %t-host.bc -fsyntax-only
+// RUN: %clang_cc1 -verify=quiet -fopenmp -x c++ -triple 
powerpc64le-unknown-linux-gnu -fopenmp-targets=powerpc64le-unknown-linux-gnu 
-emit-llvm-bc %s -o %t-host.bc

Sorry, this line was supposed to be x86_64 only, and then it no longer acts as 
a reproducer for me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64289



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

[PATCH] D64289: [OpenMP] Fix 128-bit long double support on target

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:1594
+   !Context.getTargetInfo().hasFloat128Type() &&
+   Context.getTargetInfo().getLongDoubleWidth() != 128) ||
   (Ty->isIntegerType() && Context.getTypeSize(Ty) == 128 &&

Hmm, this look strange, at least. Seems to me, in this case the size of the 
long double is 128 bit (copied from the host), but device reports that it does 
not support 128 bit double. Seems to me, it is a problem with the device 
configuration. Why does the host translate long double to 128 bit fp, while the 
device translates it to 64 bit FP?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64289



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

[PATCH] D64289: [OpenMP] Fix 128-bit long double support on target

[Joel E. Denny via Phabricator via cfe-commits]

jdenny created this revision.
jdenny added a reviewer: ABataev.
Herald added subscribers: jdoerfert, guansong.
Herald added a project: clang.

For example, without this fix, when the host is x86_64, long double is
sometimes rejected when offloading to x86_64:

  $ cat test.c
  int main() {
long double in = 0;
#pragma omp target
  in *= 2;
return 0;
  }
  $ clang -fopenmp test.c; echo $?
  0
  $ clang -fopenmp -fopenmp-targets=x86_64 test.c
  test.c:4:5: error: 'long double' is not supported on this target
  in *= 2;
  ^~
  1 error generated.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D64289

Files:
  clang/lib/Sema/SemaOpenMP.cpp
  clang/test/OpenMP/nvptx_unsupported_type_messages.cpp


Index: clang/test/OpenMP/nvptx_unsupported_type_messages.cpp
===
--- clang/test/OpenMP/nvptx_unsupported_type_messages.cpp
+++ clang/test/OpenMP/nvptx_unsupported_type_messages.cpp
@@ -1,8 +1,16 @@
 // Test target codegen - host bc file has to be created first.
-// RUN: %clang_cc1 -fopenmp -x c++ -triple x86_64-unknown-linux 
-fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm-bc %s -o %t-host.bc
+// RUN: %clang_cc1 -verify=quiet -fopenmp -x c++ -triple x86_64-unknown-linux 
-fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm-bc %s -o %t-host.bc
 // RUN: %clang_cc1 -verify -fopenmp -x c++ -triple nvptx64-unknown-unknown 
-aux-triple x86_64-unknown-linux -fopenmp-targets=nvptx64-nvidia-cuda %s 
-fopenmp-is-device -fopenmp-host-ir-file-path %t-host.bc -fsyntax-only
-// RUN: %clang_cc1 -fopenmp -x c++ -triple powerpc64le-unknown-linux-gnu 
-fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm-bc %s -o %t-host.bc
+// RUN: %clang_cc1 -verify=quiet -fopenmp -x c++ -triple 
powerpc64le-unknown-linux-gnu -fopenmp-targets=nvptx64-nvidia-cuda 
-emit-llvm-bc %s -o %t-host.bc
 // RUN: %clang_cc1 -verify -fopenmp -x c++ -triple nvptx64-unknown-unknown 
-aux-triple powerpc64le-unknown-linux-gnu -fopenmp-targets=nvptx64-nvidia-cuda 
%s -fopenmp-is-device -fopenmp-host-ir-file-path %t-host.bc -fsyntax-only
+//
+// Make sure this code does compile when the host and target are the same.
+// RUN: %clang_cc1 -verify=quiet -fopenmp -x c++ -triple x86_64-unknown-linux 
-fopenmp-targets=x86_64-unknown-linux -emit-llvm-bc %s -o %t-host.bc
+// RUN: %clang_cc1 -verify=quiet -fopenmp -x c++ -triple 
powerpc64le-unknown-linux-gnu -aux-triple x86_64-unknown-linux 
-fopenmp-targets=x86_64-unknown-linux %s -fopenmp-is-device 
-fopenmp-host-ir-file-path %t-host.bc -fsyntax-only
+// RUN: %clang_cc1 -verify=quiet -fopenmp -x c++ -triple 
powerpc64le-unknown-linux-gnu -fopenmp-targets=powerpc64le-unknown-linux-gnu 
-emit-llvm-bc %s -o %t-host.bc
+// RUN: %clang_cc1 -verify=quiet -fopenmp -x c++ -triple 
powerpc64le-unknown-linux-gnu -aux-triple powerpc64le-unknown-linux-gnu 
-fopenmp-targets=powerpc64le-unknown-linux-gnu %s -fopenmp-is-device 
-fopenmp-host-ir-file-path %t-host.bc -fsyntax-only
+
+// quiet-no-diagnostics
 
 struct T {
   char a;
Index: clang/lib/Sema/SemaOpenMP.cpp
===
--- clang/lib/Sema/SemaOpenMP.cpp
+++ clang/lib/Sema/SemaOpenMP.cpp
@@ -1590,7 +1590,8 @@
   if ((Ty->isFloat16Type() && !Context.getTargetInfo().hasFloat16Type()) ||
   ((Ty->isFloat128Type() ||
 (Ty->isRealFloatingType() && Context.getTypeSize(Ty) == 128)) &&
-   !Context.getTargetInfo().hasFloat128Type()) ||
+   !Context.getTargetInfo().hasFloat128Type() &&
+   Context.getTargetInfo().getLongDoubleWidth() != 128) ||
   (Ty->isIntegerType() && Context.getTypeSize(Ty) == 128 &&
!Context.getTargetInfo().hasInt128Type()))
 targetDiag(E->getExprLoc(), diag::err_type_unsupported)


Index: clang/test/OpenMP/nvptx_unsupported_type_messages.cpp
===
--- clang/test/OpenMP/nvptx_unsupported_type_messages.cpp
+++ clang/test/OpenMP/nvptx_unsupported_type_messages.cpp
@@ -1,8 +1,16 @@
 // Test target codegen - host bc file has to be created first.
-// RUN: %clang_cc1 -fopenmp -x c++ -triple x86_64-unknown-linux -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm-bc %s -o %t-host.bc
+// RUN: %clang_cc1 -verify=quiet -fopenmp -x c++ -triple x86_64-unknown-linux -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm-bc %s -o %t-host.bc
 // RUN: %clang_cc1 -verify -fopenmp -x c++ -triple nvptx64-unknown-unknown -aux-triple x86_64-unknown-linux -fopenmp-targets=nvptx64-nvidia-cuda %s -fopenmp-is-device -fopenmp-host-ir-file-path %t-host.bc -fsyntax-only
-// RUN: %clang_cc1 -fopenmp -x c++ -triple powerpc64le-unknown-linux-gnu -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm-bc %s -o %t-host.bc
+// RUN: %clang_cc1 -verify=quiet -fopenmp -x c++ -triple powerpc64le-unknown-linux-gnu -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm-bc %s -o %t-host.bc
 // RUN: %clang_cc1 -verify -fopenmp -x c++ -triple nvptx64-unknown-unknown -aux-triple powerpc64le-

[PATCH] D62619: [analyzer][IDF] Add a control dependency calculator + a new debug checker

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added inline comments.



Comment at: clang/include/clang/Analysis/Analyses/Dominators.h:230
+
+  virtual void releaseMemory() {
+PostDomTree.releaseMemory();

kuhar wrote:
> If the virtual function is introduced by ManagesAnalysis, isn't it better to 
> use override here?
I admit to have copy-pasted this from the class above, and, well, it isn't 
introduced by `ManagedAnalysis`. Which begs the question why it was made 
virtual at all.

Nice catch!



Comment at: clang/include/clang/Analysis/Analyses/Dominators.h:235
+  // Lazily retrieves the set of control dependencies to \p A.
+  const CFGBlockVector &getControlDependencies(CFGBlock *A) {
+auto It = ControlDepenencyMap.find(A);

kuhar wrote:
>  Can it be const?
IDFCalculator takes it's arguments by a non-const pointer. I guess I could fix 
that too on LLVM's side eventually. The method itself retrieves control 
dependencies lazily, and might modify the state of this class.



Comment at: clang/include/clang/Analysis/Analyses/Dominators.h:252
+  /// Whether \p A is control dependent on \p B.
+  bool isControlDependent(CFGBlock *A, CFGBlock *B) {
+return llvm::is_contained(getControlDependencies(A), B);

kuhar wrote:
> Can it be const?
Same.



Comment at: clang/include/clang/Analysis/Analyses/Dominators.h:257
+  // Dumps immediate control dependencies for each block.
+  void dump() {
+CFG *cfg = PostDomTree.getCFG();

NoQ wrote:
> kuhar wrote:
> > kuhar wrote:
> > > Can `dump` be const? 
> > In LLVM, `dump`s are usually annotated with the `LLVM_DUMP_METHOD` 
> > attribute and not compiled in release builds. Is the convention different 
> > in the static analyzer?
> `LLVM_DUMP_METHOD` is great. Hiding dump methods under `#ifndef NDEBUG` is 
> something i've seen very rarely. It's fairly annoying to me that exploded 
> graph dumps are unavailable in release builds, but apart from that i don't 
> have any immediate opinion, so this sounds like a global LLVM policy that 
> we're historically not paying much attention to, but i don't mind complying.
Cant be const for the same reasons.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D62619



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

[PATCH] D63279: [Analyzer] Unroll for-loops where the upper boundary is a variable with know value

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added inline comments.



Comment at: lib/StaticAnalyzer/Core/LoopUnrolling.cpp:214-216
+  const Expr *BoundExpr = CondOp->getLHS()->IgnoreParenImpCasts();
+  if (BoundExpr == Matches[0].getNodeAs("boundVarOperand"))
+BoundExpr = CondOp->getRHS()->IgnoreParenImpCasts();

Alright, I stood on top of this for longer than I'd like to admit, what's 
happening here? Like, `CondExpr`, with the given testfile, should be `i < n`, 
right? Then `BoundExpr` is supposed to be `i`, and if that is equal to 
`Matches[0].getNodeAs("boundVarOperand")` (which I guess is supposed to 
`n`), which I'm not sure how it could ever happen, we assign the RHS of the 
assignment to `BoundExpr`?

What does this do? Why is it needed? Which 2 test cases cover these lines?



Comment at: lib/StaticAnalyzer/Core/LoopUnrolling.cpp:221-230
+  if (BoundNumVal.isUnknown()) {
+if (const auto *BoundDeclRefExpr = dyn_cast(BoundExpr)) {
+  // FIXME: Add other declarations such as Objective-C fields
+  if (const auto *BoundVarDecl =
+  dyn_cast(BoundDeclRefExpr->getDecl())) {
+BoundNumVal = State->getSVal(
+State->getLValue(BoundVarDecl, Pred->getLocationContext()));

I don't see obvious test case for which `BoundNumVal` would be unknown, am I 
wrong?


Repository:
  rC Clang

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

https://reviews.llvm.org/D63279



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

[PATCH] D64274: [analyzer] VirtualCallChecker overhaul.

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus requested changes to this revision.
Szelethus added a comment.
This revision now requires changes to proceed.

I happen to have very recent analyses on a couple projects, I'll throw this in: 
LLVM+Clang+Clang-tools-extra 
.
 No findings on Xerces or Bitcoin.

I caught up on the thread you linked, and the checker code since it wasn't that 
long. Overall, what you'd like to do with this checker sounds amazing! I agree 
that the visitor does more harm then good, and that pure only analysis should 
be on-by-default. Also, thanks for cleaning up the testfile!!

> I'd like to point out that it breaks backwards compatibility. I'm not going 
> to be hurt by this and i haven't heard a lot about users of this opt-in check 
> but if you know them, please let me know. If we agree to break backwards 
> compatibility, we should make sure that the result is the best, so i'd like 
> to hear @Szelethus's opinion, as he usually has strong opinions on checker 
> options and dependencies :)

CodeChecker turns on `optin.cplusplus.VirtualCall` by default, so this wouldn't 
hurt its users much. Also, you can enable/disable checkers from an arbitrary 
version of clang, their names aren't hardcoded (only the on-by-default list).

That said, dependencywise, there is a directed circle here, despite 
`optin.cplusplus.VirtualCall` being a glorified checker option (this is what I 
like to label as a "subchecker"). [side note: since this checker doesn't really 
do any modeling, in fact it wouldn even store a state if it inspected the call 
stack, it wouldn't be logical to create a modeling checker that would be a 
dependency of both `PureVirtualCall` and `VirtualCall`. Since `PureVirtualCall` 
is a strict subset of `VirtualCall`, if it causes any crashes, you have to 
disable both anyways. ] Your current implementation would cause an assertion 
failure if you enabled both checkers due to attempting to register the checker 
object 2 times, to avoid that, *see inlines*

   <--
 /\
  PureVirtualCallVirtualCall and should be: PureVirtualCall 
<-- VirtualCall
 \/
   -->






Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:562
+  HelpText<"Check virtual function calls during construction/destruction">,
   Documentation;
 

`Dependencies<[PureVirtualCallChecker]>,`



Comment at: clang/lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp:210
 void ento::registerVirtualCallChecker(CheckerManager &mgr) {
   VirtualCallChecker *checker = mgr.registerChecker();
+  checker->IsPureOnly = false;

`auto *Checker = mgr.getChecker();`


Repository:
  rC Clang

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

https://reviews.llvm.org/D64274



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

[PATCH] D64232: [analyzer] Prune calls to functions with linear CFGs that return a non-zero constrained value

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus updated this revision to Diff 208276.
Szelethus added a comment.

Rebase.


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

https://reviews.llvm.org/D64232

Files:
  clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
  clang/test/Analysis/diagnostics/find_last_store.c
  clang/test/Analysis/track-control-dependency-conditions.cpp
  clang/test/Analysis/uninit-vals.c

Index: clang/test/Analysis/uninit-vals.c
===
--- clang/test/Analysis/uninit-vals.c
+++ clang/test/Analysis/uninit-vals.c
@@ -149,8 +149,6 @@
   RetVoidFuncType f = foo_radar12278788_fp;
   return ((RetIntFuncType)f)(); //expected-warning {{Undefined or garbage value returned to caller}}
 //expected-note@-1 {{Undefined or garbage value returned to caller}}
-//expected-note@-2 {{Calling 'foo_radar12278788_fp'}}
-//expected-note@-3 {{Returning from 'foo_radar12278788_fp'}}
 }
 
 void rdar13665798() {
@@ -164,8 +162,6 @@
 RetVoidFuncType f = foo_radar12278788_fp;
 return ((RetIntFuncType)f)(); //expected-warning {{Undefined or garbage value returned to caller}}
   //expected-note@-1 {{Undefined or garbage value returned to caller}}
-  //expected-note@-2 {{Calling 'foo_radar12278788_fp'}}
-  //expected-note@-3 {{Returning from 'foo_radar12278788_fp'}}
   }();
 }
 
@@ -182,18 +178,14 @@
 void use(struct Point p); 
 
 void testUseHalfPoint() {
-  struct Point p = getHalfPoint(); // expected-note{{Calling 'getHalfPoint'}}
-   // expected-note@-1{{Returning from 'getHalfPoint'}}
-   // expected-note@-2{{'p' initialized here}}
+  struct Point p = getHalfPoint(); // expected-note{{'p' initialized here}}
   use(p); // expected-warning{{uninitialized}}
   // expected-note@-1{{uninitialized}}
 }
 
 void testUseHalfPoint2() {
   struct Point p;
-  p = getHalfPoint(); // expected-note{{Calling 'getHalfPoint'}}
-  // expected-note@-1{{Returning from 'getHalfPoint'}}
-  // expected-note@-2{{Value assigned to 'p'}}
+  p = getHalfPoint(); // expected-note{{Value assigned to 'p'}}
   use(p); // expected-warning{{uninitialized}}
   // expected-note@-1{{uninitialized}}
 }
Index: clang/test/Analysis/track-control-dependency-conditions.cpp
===
--- clang/test/Analysis/track-control-dependency-conditions.cpp
+++ clang/test/Analysis/track-control-dependency-conditions.cpp
@@ -119,7 +119,7 @@
 bool coin();
 
 bool foo() {
-  return coin(); // tracking-note{{Returning value}}
+  return coin();
 }
 
 int bar;
@@ -127,11 +127,9 @@
 void test() {
   int *x = 0; // expected-note{{'x' initialized to a null pointer value}}
 
-  if (int flag = foo()) // tracking-note{{Calling 'foo'}}
-// tracking-note@-1{{Returning from 'foo'}}
-// debug-note@-2{{Tracking condition 'flag'}}
-// expected-note@-3{{Assuming 'flag' is not equal to 0}}
-// expected-note@-4{{Taking true branch}}
+  if (int flag = foo()) // debug-note{{Tracking condition 'flag'}}
+// expected-note@-1{{Assuming 'flag' is not equal to 0}}
+// expected-note@-2{{Taking true branch}}
 
 *x = 5; // expected-warning{{Dereference of null pointer}}
 // expected-note@-1{{Dereference of null pointer}}
@@ -142,18 +140,16 @@
 bool coin();
 
 struct ConvertsToBool {
-  operator bool() const { return coin(); } // tracking-note{{Returning value}}
+  operator bool() const { return coin(); }
 };
 
 void test() {
   int *x = 0; // expected-note{{'x' initialized to a null pointer value}}
 
   if (ConvertsToBool())
-// tracking-note@-1 {{Calling 'ConvertsToBool::operator bool'}}
-// tracking-note@-2{{Returning from 'ConvertsToBool::operator bool'}}
-// debug-note@-3{{Tracking condition 'ConvertsToBool()'}}
-// expected-note@-4{{Assuming the condition is true}}
-// expected-note@-5{{Taking true branch}}
+// debug-note@-1{{Tracking condition 'ConvertsToBool()'}}
+// expected-note@-2{{Assuming the condition is true}}
+// expected-note@-3{{Taking true branch}}
 *x = 5; // expected-warning{{Dereference of null pointer}}
 // expected-note@-1{{Dereference of null pointer}}
 }
@@ -163,18 +159,16 @@
 namespace unimportant_returning_value_note {
 bool coin();
 
-bool flipCoin() { return coin(); } // tracking-note{{Returning value}}
+bool flipCoin() { return coin(); }
 
 void i(int *ptr) {
   if (ptr) // expected-note{{Assuming 'ptr' is null}}
// expected-note@-1{{Taking false branch}}
 ;
   if (!flipCoin())
-// tracking-note@-1{{Calling 'flipCoin'}}
-// t

[PATCH] D64272: [analyzer] Note last writes to a condition only in a nested stackframe

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus updated this revision to Diff 208275.
Szelethus added a comment.

Rebase after D64271  being abandoned.


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

https://reviews.llvm.org/D64272

Files:
  clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
  clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
  clang/test/Analysis/track-control-dependency-conditions.cpp

Index: clang/test/Analysis/track-control-dependency-conditions.cpp
===
--- clang/test/Analysis/track-control-dependency-conditions.cpp
+++ clang/test/Analysis/track-control-dependency-conditions.cpp
@@ -129,10 +129,9 @@
 
   if (int flag = foo()) // tracking-note{{Calling 'foo'}}
 // tracking-note@-1{{Returning from 'foo'}}
-// tracking-note@-2{{'flag' initialized here}}
-// debug-note@-3{{Tracking condition 'flag'}}
-// expected-note@-4{{Assuming 'flag' is not equal to 0}}
-// expected-note@-5{{Taking true branch}}
+// debug-note@-2{{Tracking condition 'flag'}}
+// expected-note@-3{{Assuming 'flag' is not equal to 0}}
+// expected-note@-4{{Taking true branch}}
 
 *x = 5; // expected-warning{{Dereference of null pointer}}
 // expected-note@-1{{Dereference of null pointer}}
@@ -211,7 +210,7 @@
 int getInt();
 
 void f() {
-  int flag = getInt(); // tracking-note{{'flag' initialized here}}
+  int flag = getInt();
   int *x = 0; // expected-note{{'x' initialized to a null pointer value}}
   if (flag) // expected-note{{Assuming 'flag' is not equal to 0}}
 // expected-note@-1{{Taking true branch}}
@@ -226,9 +225,8 @@
 int getInt();
 
 void f(int y) {
-  y = 1; // tracking-note{{The value 1 is assigned to 'y'}}
-  flag = y; // tracking-note{{The value 1 is assigned to 'flag'}}
-
+  y = 1;
+  flag = y;
   int *x = 0; // expected-note{{'x' initialized to a null pointer value}}
   if (flag) // expected-note{{'flag' is 1}}
 // expected-note@-1{{Taking true branch}}
@@ -244,7 +242,7 @@
 
 void foo() {
   int y;
-  y = 1; // tracking-note{{The value 1 is assigned to 'y'}}
+  y = 1;
   flag = y; // tracking-note{{The value 1 is assigned to 'flag'}}
 
 }
@@ -277,7 +275,7 @@
 
   foo(); // tracking-note{{Calling 'foo'}}
  // tracking-note@-1{{Returning from 'foo'}}
-  y = flag; // tracking-note{{Value assigned to 'y'}}
+  y = flag;
 
   if (y) // expected-note{{Assuming 'y' is not equal to 0}}
  // expected-note@-1{{Taking true branch}}
@@ -326,7 +324,7 @@
 void f(int flag) {
   int *x = 0; // expected-note{{'x' initialized to a null pointer value}}
 
-  flag = getInt(); // tracking-note{{Value assigned to 'flag'}}
+  flag = getInt();
   assert(flag); // tracking-note{{Calling 'assert'}}
 // tracking-note@-1{{Returning from 'assert'}}
 
Index: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===
--- clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -1421,6 +1421,7 @@
 
   if (!StoreSite)
 return nullptr;
+
   Satisfied = true;
 
   // If we have an expression that provided the value, try to track where it
@@ -1429,10 +1430,14 @@
 if (!IsParam)
   InitE = InitE->IgnoreParenCasts();
 
-bugreporter::trackExpressionValue(StoreSite, InitE, BR,
-  EnableNullFPSuppression);
+::trackExpressionValue(StoreSite, InitE, BR,
+   EnableNullFPSuppression, TKind);
   }
 
+  if (TKind == TrackingKind::ConditionTracking &&
+  StoreSite->getStackFrame() == OriginSFC)
+return nullptr;
+
   // Okay, we've found the binding. Emit an appropriate message.
   SmallString<256> sbuf;
   llvm::raw_svector_ostream os(sbuf);
@@ -1458,7 +1463,7 @@
   if (const VarRegion *OriginalR = BDR->getOriginalRegion(VR)) {
 if (auto KV = State->getSVal(OriginalR).getAs())
   BR.addVisitor(llvm::make_unique(
-  *KV, OriginalR, EnableNullFPSuppression, TKind));
+  *KV, OriginalR, EnableNullFPSuppression, TKind, OriginSFC));
   }
 }
   }
@@ -1890,6 +1895,7 @@
 return false;
 
   ProgramStateRef LVState = LVNode->getState();
+  const StackFrameContext *SFC = LVNode->getStackFrame();
 
   // We only track expressions if we believe that they are important. Chances
   // are good that control dependencies to the tracking point are also improtant
@@ -1925,7 +1931,7 @@
 if (RR && !LVIsNull)
   if (auto KV = LVal.getAs())
 report.addVisitor(llvm::make_unique(
-  *KV, RR, EnableNullFPSuppression, TKind));
+  *KV, RR, EnableNullFPSuppression, TKind, SFC));
 
 // I

[PATCH] D64271: [analyzer] Don't track the right hand side of the last store for conditions

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus abandoned this revision.
Szelethus added a comment.

You're right. If condition tracking only adds necessary information anyways, 
this shouldn't hurt that much anyways.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64271



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

[PATCH] D64287: [analyzer] Track the right hand side of the last store unconditionally

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus created this revision.
Szelethus added reviewers: NoQ, xazax.hun, rnkovacs, baloghadamsoftware, 
Charusso.
Szelethus added a project: clang.
Herald added subscribers: cfe-commits, gamesh411, dkrupp, donat.nagy, 
mikhail.ramalho, a.sidorin, szepet, whisperity.

The following code snippet taken from D64271#1572188 
 has an issue: namely, because 
`flag`'s value isn't undef or a concrete int, it isn't being tracked.

  int flag;
  bool coin();
  
  void foo() {
flag = coin();
  }
  
  void test() {
int *x = 0;
int local_flag;
flag = 1;
  
foo();
local_flag = flag;
if (local_flag)
  x = new int;
  
foo();
local_flag = flag;
if (local_flag)
  *x = 5;
  }

This, in my opinion, makes no sense, other values may be interesting too. 
Originally added by rC185608 .


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D64287

Files:
  clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
  clang/test/Analysis/track-control-dependency-conditions.cpp
  clang/test/Analysis/uninit-const.c
  clang/test/Analysis/uninit-const.cpp

Index: clang/test/Analysis/uninit-const.cpp
===
--- clang/test/Analysis/uninit-const.cpp
+++ clang/test/Analysis/uninit-const.cpp
@@ -66,8 +66,8 @@
 
 void f6_2(void) {
   int t;   //expected-note {{'t' declared without an initial value}}
-  int &p = t;
-  int &s = p;
+  int &p = t;  //expected-note {{'p' initialized here}}
+  int &s = p;  //expected-note {{'s' initialized here}}
   int &q = s;  //expected-note {{'q' initialized here}}
   doStuff6(q); //expected-warning {{1st function call argument is an uninitialized value}}
//expected-note@-1 {{1st function call argument is an uninitialized value}}
@@ -96,7 +96,7 @@
 
 
 void f5(void) {
-  int t;
+  int t; // expected-note {{'t' declared without an initial value}}
   int* tp = &t;// expected-note {{'tp' initialized here}}
   doStuff_uninit(tp);  // expected-warning {{1st function call argument is a pointer to uninitialized value}}
// expected-note@-1 {{1st function call argument is a pointer to uninitialized value}}
Index: clang/test/Analysis/uninit-const.c
===
--- clang/test/Analysis/uninit-const.c
+++ clang/test/Analysis/uninit-const.c
@@ -24,15 +24,15 @@
 void doStuff_variadic(const int *u, ...){};
 
 void f_1(void) {
-  int t;
+  int t; // expected-note {{'t' declared without an initial value}}
   int* tp = &t;// expected-note {{'tp' initialized here}}
   doStuff_pointerToConstInt(tp);  // expected-warning {{1st function call argument is a pointer to uninitialized value}}
// expected-note@-1 {{1st function call argument is a pointer to uninitialized value}}
 }
 
 void f_1_1(void) {
-  int t;
-  int* tp1 = &t;
+  int t; // expected-note {{'t' declared without an initial value}}
+  int* tp1 = &t; // expected-note {{'tp1' initialized here}}
   int* tp2 = tp1;// expected-note {{'tp2' initialized here}}
   doStuff_pointerToConstInt(tp2);  // expected-warning {{1st function call argument is a pointer to uninitialized value}}
// expected-note@-1 {{1st function call argument is a pointer to uninitialized value}}
@@ -40,12 +40,15 @@
 
 
 int *f_2_sub(int *p) {
-  return p;
+  return p; // expected-note {{Returning pointer (loaded from 'p')}}
 }
 
 void f_2(void) {
-  int t;
-  int* p = f_2_sub(&t);
+  int t; // expected-note {{'t' declared without an initial value}}
+  int* p = f_2_sub(&t); // expected-note {{Passing value via 1st parameter 'p'}}
+// expected-note@-1{{Calling 'f_2_sub'}}
+// expected-note@-2{{Returning from 'f_2_sub'}}
+// expected-note@-3{{'p' initialized here}}
   int* tp = p; // expected-note {{'tp' initialized here}}
   doStuff_pointerToConstInt(tp); // expected-warning {{1st function call argument is a pointer to uninitialized value}}
   // expected-note@-1 {{1st function call argument is a pointer to uninitialized value}}
@@ -62,7 +65,7 @@
 }
 
 void f_5(void) {
-  int ta[5];
+  int ta[5]; // expected-note {{'ta' initialized here}}
   int* tp = ta;// expected-note {{'tp' initialized here}}
   doStuff_pointerToConstInt(tp);  // expected-warning {{1st function call argument is a pointer to uninitialized value}}
// expected-note@-1 {{1st function call argument is a pointer to uninitialized value}}
@@ -99,7 +102,7 @@
 }
 
 void f_9(void) {
-  int  a[6];
+  int  a[6]; // expected-note {{'a' initialized here}}
   int const *ptau = a; // expected-note {{'ptau' initialized here}}
   doStuff_arrayOfConstInt(ptau);// expected-warning {{1st function call argument is a pointer to uninitialized value}}
  

r365264 - [Rewrite] Try to fix buildbot link fail left by r365263

[Joel E. Denny via cfe-commits]

Author: jdenny
Date: Sat Jul  6 09:28:32 2019
New Revision: 365264

URL: http://llvm.org/viewvc/llvm-project?rev=365264&view=rev
Log:
[Rewrite] Try to fix buildbot link fail left by r365263

http://lab.llvm.org:8011/builders/clang-ppc64le-linux-multistage/builds/10272

Modified:
cfe/trunk/unittests/Rewrite/CMakeLists.txt

Modified: cfe/trunk/unittests/Rewrite/CMakeLists.txt
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Rewrite/CMakeLists.txt?rev=365264&r1=365263&r2=365264&view=diff
==
--- cfe/trunk/unittests/Rewrite/CMakeLists.txt (original)
+++ cfe/trunk/unittests/Rewrite/CMakeLists.txt Sat Jul  6 09:28:32 2019
@@ -10,5 +10,6 @@ clang_target_link_libraries(RewriteTests
   PRIVATE
   clangFrontend
   clangRewrite
+  clangSerialization
   clangTooling
   )


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

[PATCH] D60335: Use -fomit-frame-pointer when optimizing PowerPC code

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.
Herald added a subscriber: wuzish.

I would write the following test (I can commit that for you)

  diff --git c/test/Driver/frame-pointer-elim.c 
w/test/Driver/frame-pointer-elim.c
  index 6fcd3eb75a..c2e9ccb225 100644
  --- c/test/Driver/frame-pointer-elim.c
  +++ w/test/Driver/frame-pointer-elim.c
  @@ -1 +1,3 @@
  +// DISABLE-FP-ELIM: "-mdisable-fp-elim"
  +
   // For these next two tests when optimized we should omit the leaf frame
  @@ -31,4 +33,3 @@
   // RUN: %clang -### -target i386-apple-darwin -S %s 2>&1 | \
  -// RUN:   FileCheck --check-prefix=DARWIN %s
  -// DARWIN: "-mdisable-fp-elim"
  +// RUN:   FileCheck --check-prefix=DISABLE-FP-ELIM %s
   
  @@ -71,2 +72,7 @@
   
  +// RUN: %clang -### -target powerpc64 -S %s 2>&1 | \
  +// RUN:   FileCheck --check-prefix=DISABLE-FP-ELIM %s
  +// RUN: %clang -### -target powerpc64 -S -O1 %s 2>&1 | \
  +// RUN:   FileCheck --check-prefix=OMIT_ALL %s
  +
   void f0() {}

but I don't understand why OpenBSD enabled frame pointer elimination in 
rL358775  then reverted it in rL364679 
. It is important to know whether OpenBSD 
needs frame pointer elimination.


Repository:
  rC Clang

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

https://reviews.llvm.org/D60335



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

[PATCH] D63845: [WIP] Create a clang attribute that lets users specify LLVM attributes

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D63845#1571855 , @jdoerfert wrote:

> I think we should postpone a detailed discussion until we are ready to send 
> an RFC on this. Nevertheless, I inlined some responses below.
>
> In D63845#1570639 , @aaron.ballman 
> wrote:
>
> > In D63845#1566987 , @jdoerfert 
> > wrote:
> >
> > > In D63845#1561983 , @lebedev.ri 
> > > wrote:
> > >
> > > > In D63845#1561793 , @jdoerfert 
> > > > wrote:
> > > >
> > > > > In D63845#1560605 , 
> > > > > @aaron.ballman wrote:
> > > > >
> > > > > > In D63845#1559995 , 
> > > > > > @lebedev.ri wrote:
> > > > > >
> > > > > > > What's the target use-case here? What can't be solved with normal 
> > > > > > > attributes?
> > > > > >
> > > > >
> > > > >
> > > > > With "normal" you mean something like `__attribute__((noescape))`? We 
> > > > > don't have them for all attributes, I doubt we want to but I might be 
> > > > > wrong.
> > > >
> > > >
> > > > That is precisely the question.
> > > >  What is the motivation for exposing such LLVM-specific low-level 
> > > > unstable implementation detail?
> > >
> > >
> > > I would disagree to the unstable part. Keeping LLVM-IR backward 
> > > compatibility is always a priority and changing the meaning of an 
> > > existing LLVM-IR attribute will break various things already.
> > >  Why would you think it is unstable?
> >
> >
> > Because, to date, we've never made blanket any stability guarantees about 
> > LLVM attributes from within the context of Clang. Instead, LLVM attributes 
> > are introduced into Clang on a case by case basis when they've been 
> > determined to be stable enough to warrant exposing. This extra layer also 
> > gives us more flexibility in translating frontend attributes into backend 
> > attributes.
>
>
> You still assume LLVM attributes are not stable while I tried to argue they 
> are, they have to be already for backward compatibility, at least target 
> agnostic ones.


I think *many* LLVM attributes have stability to them, especially once they've 
had some time to bake. It's the odd ones that don't fit the norm that have me 
uncomfortable. However, because the proposal isn't a blanket addition of *all* 
LLVM attributes automatically, I'm far less concerned. I didn't pick up on that 
bit from the original proposal.

> Your example below was `thunk` and you mentioned other problems that can 
> arise but I think we should step back for a moment and talk about the use 
> case here because that should limit the potential problems. For me the use 
> case is as follows: The user, or a tool, wants to encode additional 
> information in the source to aide optimization. The basically only way to do 
> this in IR is to add attributes to one of three locations, (1) function 
> level, (2) return value level, or (3) argument level. For all three we have 
> examples in Clang so we know how do interact with templates, and other 
> potential hurdles. Now it is not the goal to allow all attributes as some of 
> them encode not information but implementation details, e.g., `thunk`. I'm 
> basically interested in attributes that can be safely derived and dropped in 
> he IR today.

That seems like a reasonable decision, to me. I picked on `thunk` only because 
it was a good example of an attribute that would require additional semantic 
checks that couldn't be easily automated, but I'm sure there's a subset of LLVM 
attributes that have easier semantics to deal with. Thank you for the extra 
clarification.

>>> Also, we basically do expose the low-level parts one by one through 
>>> "normal" attributes as soon as someone has enough motivation to write all 
>>> the necessary boilerplate (see more below). Why not merge all the 
>>> logic/code-gen into a single uniform framework that deals with LLVM-IR 
>>> attributes?
>> 
>> Because that increases the chance of bad user experiences by lowering the 
>> bar that ensures newly added attributes behave well for user code. That 
>> boilerplate exists for a reason and forces a developer to make decisions 
>> (like what number and kind of arguments are accepted, what spellings make 
>> sense for the attribute, what subjects the attribute appertains to, etc).
> 
> We can still make all these decisions, this patch almost makes all of them 
> explicit. We can whitelist arguments (through the switch that exists), we can 
> rename them between the FE and IR if we want to (though I would prefer not 
> to), and we do restrict the positions already to the three interesting ones.

Fantastic! I think that if you take the tablegen approach I suggested, we can 
hopefully (easily) cover all the random bits of automated checking we've added 
for frontend attribute checking.

>>> In D638

r365263 - [Rewrite] Try to fix buildbot link fail caused by r365258

[Joel E. Denny via cfe-commits]

Author: jdenny
Date: Sat Jul  6 06:44:57 2019
New Revision: 365263

URL: http://llvm.org/viewvc/llvm-project?rev=365263&view=rev
Log:
[Rewrite] Try to fix buildbot link fail caused by r365258

http://lab.llvm.org:8011/builders/clang-ppc64le-linux-multistage/builds/10270

Modified:
cfe/trunk/unittests/Rewrite/CMakeLists.txt

Modified: cfe/trunk/unittests/Rewrite/CMakeLists.txt
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Rewrite/CMakeLists.txt?rev=365263&r1=365262&r2=365263&view=diff
==
--- cfe/trunk/unittests/Rewrite/CMakeLists.txt (original)
+++ cfe/trunk/unittests/Rewrite/CMakeLists.txt Sat Jul  6 06:44:57 2019
@@ -8,6 +8,7 @@ add_clang_unittest(RewriteTests
   )
 clang_target_link_libraries(RewriteTests
   PRIVATE
+  clangFrontend
   clangRewrite
   clangTooling
   )


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

[PATCH] D64062: Remove both -dumpversion and __VERSION__

[Sylvestre Ledru via Phabricator via cfe-commits]

sylvestre.ledru added a comment.

@rnk is it good for you ?


Repository:
  rC Clang

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

https://reviews.llvm.org/D64062



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

[PATCH] D64277: [X86][PowerPC] Support -mlong-double-128

[Fangrui Song via Phabricator via cfe-commits]

MaskRay marked an inline comment as done.
MaskRay added inline comments.



Comment at: test/OpenMP/nvptx_unsupported_type_codegen.cpp:79
 }
-
-// CHECK: define weak void 
@__omp_offloading_{{.+}}foo{{.+}}_l75([[BIGTYPE:.+]]*

ABataev wrote:
> MaskRay wrote:
> > ABataev wrote:
> > > MaskRay wrote:
> > > > @ABataev The mangling scheme of __float128 is broken on x86 before this 
> > > > patch. For some reason the two lines no longer apply.. I assume you can 
> > > > fix this test properly :)
> > > You mean, these lines are not generated anymore? If so, it means that you 
> > > broke compatibility between the host and device codegen with this patch. 
> > The two lines are not generated. Can you help fix the test? The mangling 
> > scheme of __float128 is incorrect without this patch. If the test relies on 
> > that, it should be fixed. I know little about openmp+nvidia to understand 
> > what's going on..
> Sure, will try to send additional changes next week, most probably on Monday. 
> Does this work for you?
Thanks!


Repository:
  rC Clang

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

https://reviews.llvm.org/D64277



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

[PATCH] D64277: [X86][PowerPC] Support -mlong-double-128

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: test/OpenMP/nvptx_unsupported_type_codegen.cpp:79
 }
-
-// CHECK: define weak void 
@__omp_offloading_{{.+}}foo{{.+}}_l75([[BIGTYPE:.+]]*

MaskRay wrote:
> ABataev wrote:
> > MaskRay wrote:
> > > @ABataev The mangling scheme of __float128 is broken on x86 before this 
> > > patch. For some reason the two lines no longer apply.. I assume you can 
> > > fix this test properly :)
> > You mean, these lines are not generated anymore? If so, it means that you 
> > broke compatibility between the host and device codegen with this patch. 
> The two lines are not generated. Can you help fix the test? The mangling 
> scheme of __float128 is incorrect without this patch. If the test relies on 
> that, it should be fixed. I know little about openmp+nvidia to understand 
> what's going on..
Sure, will try to send additional changes next week, most probably on Monday. 
Does this work for you?


Repository:
  rC Clang

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

https://reviews.llvm.org/D64277



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

[PATCH] D64277: [X86][PowerPC] Support -mlong-double-128

[Fangrui Song via Phabricator via cfe-commits]

MaskRay marked an inline comment as done.
MaskRay added inline comments.



Comment at: test/OpenMP/nvptx_unsupported_type_codegen.cpp:79
 }
-
-// CHECK: define weak void 
@__omp_offloading_{{.+}}foo{{.+}}_l75([[BIGTYPE:.+]]*

ABataev wrote:
> MaskRay wrote:
> > @ABataev The mangling scheme of __float128 is broken on x86 before this 
> > patch. For some reason the two lines no longer apply.. I assume you can fix 
> > this test properly :)
> You mean, these lines are not generated anymore? If so, it means that you 
> broke compatibility between the host and device codegen with this patch. 
The two lines are not generated. Can you help fix the test? The mangling scheme 
of __float128 is incorrect without this patch. If the test relies on that, it 
should be fixed. I know little about openmp+nvidia to understand what's going 
on..


Repository:
  rC Clang

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

https://reviews.llvm.org/D64277



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

[PATCH] D64284: (WIP) share template instantiations from PCH in one .o if -building-pch-with-obj

[Luboš Luňák via Phabricator via cfe-commits]

llunak created this revision.
llunak added reviewers: rsmith, dblaikie, hans.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

This is a WIP patch that can significantly reduce build time by avoiding 
needlessly repeatedly instantiating templates when using PCH.

As described in http://lists.llvm.org/pipermail/cfe-dev/2019-May/062371.html / 
http://llunak.blogspot.com/2019/05/why-precompiled-headers-do-not-improve.html 
, whenever a compilation uses PCH it instantiates every template that's been 
instantiated in the PCH, which can add significant build time. This patch 
allows avoiding that, if -building-pch-with-obj option is used, the templates 
will be instantiated just once in the object file that should accompany the 
PCH, and then it can be skipped during normal compilations.

Testcase: I've built an entire debug build of LibreOffice with this patch and 
it works.

Pending problems/questions:

- The patch currently skips C++ constructors and destructors, because (at least 
with the Itanium ABI) they can be emitted in several variants (complete vs base 
ctor/dtor) and I don't know how to force emitting all variants. Ctors/dtors are 
a significant portion of PCH template instantiations, so without this the 
building time improvement is not as large as it could be.

- The patch currently does not handle function inlining well (=it works only 
with -O0 builds). This optimization cannot be done with template functions that 
should be inlined by the compilation, so the code needs to check whether a 
function would possibly be inlined. The problem is that this depends on 
settings in CodeGenOptions and Sema (understandably) doesn't have an access to 
that. How can I access that in Sema? Does it make sense to try to handle this, 
or is the deciding whether a function will get actually inlined too complex and 
this should rather be skipped for CodeGen::NormalInlining?

- In rate circumstances it is possible to get undefined references because of 
instantiations in the PCH's object file. Specifically this happens if the PCH 
includes a header providing non-exported code from another module (=shared 
library), in that case the PCH may refer to it. I think this is conceptually 
not possible to handle in Clang, either the code should be cleaned up to not 
provide non-exported code to other modules, or it's possible to use linker's 
--gc-sections to drop such instantiations.

- In Sema::InstantiateFunctionDefinition() the code for extern templates still 
instantiates a function if it has getContainedAutoType(), so my code should 
probably also check that. But I'm not sure what that is (is that 'auto foo() { 
return 1; }' ?) or why that would need an instance in every TU.

- Is there a guideline on when to use data members in class Decl vs functions? 
The patch uses DeclIsPendingInstantiationFromPCHWithObjectFile() to check 
whether a FunctionDecl comes from the PCH, which may get called quite often. 
This could be optimized away by storing a flag in class Decl.


Repository:
  rC Clang

https://reviews.llvm.org/D64284

Files:
  clang/include/clang/AST/ExternalASTSource.h
  clang/include/clang/Sema/MultiplexExternalSemaSource.h
  clang/include/clang/Serialization/ASTReader.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/Sema/MultiplexExternalSemaSource.cpp
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/lib/Serialization/ASTReader.cpp

Index: clang/lib/Serialization/ASTReader.cpp
===
--- clang/lib/Serialization/ASTReader.cpp
+++ clang/lib/Serialization/ASTReader.cpp
@@ -8293,14 +8293,14 @@
 
 void ASTReader::ReadPendingInstantiations(
SmallVectorImpl> &Pending) {
-  for (unsigned Idx = 0, N = PendingInstantiations.size(); Idx < N;) {
+  for (unsigned Idx = PendingInstantiationsReadCount, N = PendingInstantiations.size(); Idx < N;) {
 ValueDecl *D = cast(GetDecl(PendingInstantiations[Idx++]));
 SourceLocation Loc
   = SourceLocation::getFromRawEncoding(PendingInstantiations[Idx++]);
 
 Pending.push_back(std::make_pair(D, Loc));
   }
-  PendingInstantiations.clear();
+  PendingInstantiationsReadCount = PendingInstantiations.size();
 }
 
 void ASTReader::ReadLateParsedTemplates(
@@ -8522,6 +8522,17 @@
   return MF && MF->PCHHasObjectFile;
 }
 
+bool ASTReader::DeclIsPendingInstantiationFromPCHWithObjectFile(const Decl *D) {
+  if( !DeclIsFromPCHWithObjectFile(D))
+return false;
+  for (unsigned Idx = 0, N = PendingInstantiations.size(); Idx < N;) {
+if( D == cast(GetDecl(PendingInstantiations[Idx++])))
+return true;
+Idx++;
+  }
+  return false;
+}
+
 ModuleFile *ASTReader::getLocalModuleFile(ModuleFile &F, unsigned ID) {
   if (ID & 1) {
 // It's a module, look it up by submodule ID.
Index: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
===
--- clang/lib/Sema/SemaTemplateInstantiateDecl.cpp

[PATCH] D64277: [X86][PowerPC] Support -mlong-double-128

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: test/OpenMP/nvptx_unsupported_type_codegen.cpp:79
 }
-
-// CHECK: define weak void 
@__omp_offloading_{{.+}}foo{{.+}}_l75([[BIGTYPE:.+]]*

MaskRay wrote:
> @ABataev The mangling scheme of __float128 is broken on x86 before this 
> patch. For some reason the two lines no longer apply.. I assume you can fix 
> this test properly :)
You mean, these lines are not generated anymore? If so, it means that you broke 
compatibility between the host and device codegen with this patch. 


Repository:
  rC Clang

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

https://reviews.llvm.org/D64277



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

[PATCH] D64277: [X86][PowerPC] Support -mlong-double-128

[Fangrui Song via Phabricator via cfe-commits]

MaskRay marked an inline comment as done.
MaskRay added a subscriber: ABataev.
MaskRay added inline comments.



Comment at: test/OpenMP/nvptx_unsupported_type_codegen.cpp:79
 }
-
-// CHECK: define weak void 
@__omp_offloading_{{.+}}foo{{.+}}_l75([[BIGTYPE:.+]]*

@ABataev The mangling scheme of __float128 is broken on x86 before this patch. 
For some reason the two lines no longer apply.. I assume you can fix this test 
properly :)


Repository:
  rC Clang

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

https://reviews.llvm.org/D64277



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

[PATCH] D64283: [PowerPC] Support -mabi=ieeelongdouble and -mabi=ibmlongdouble

[Fangrui Song via Phabricator via cfe-commits]

MaskRay created this revision.
MaskRay added reviewers: awilfox, dalias, echristo, hfinkel, nemanjai, jsji, 
kbarton.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
MaskRay added parent revisions: D64282: [PowerPC] Support fp128 libcalls, 
D64277: [X86][PowerPC] Support -mlong-double-128.

In -mlong-double-128 mode (default on Linux glibc),

- -mabi=ibmlongdouble (default): use IBM extended double format for long double
- -mabi=ieeelongdouble: use IEEE 754 quadruple-precision format for long double


Repository:
  rC Clang

https://reviews.llvm.org/D64283

Files:
  include/clang/Basic/LangOptions.def
  include/clang/Driver/Options.td
  lib/Basic/Targets/PPC.cpp
  lib/Driver/ToolChains/Clang.cpp
  lib/Frontend/CompilerInvocation.cpp
  test/CodeGen/ppc64-long-double.cpp
  test/Driver/ppc-abi.c

Index: test/Driver/ppc-abi.c
===
--- test/Driver/ppc-abi.c
+++ test/Driver/ppc-abi.c
@@ -66,4 +66,20 @@
 // CHECK-ELFv2-PIC: "-mrelocation-model" "pic" "-pic-level" "2"
 // CHECK-ELFv2-PIC: "-target-abi" "elfv2"
 
+// Check -mabi=ieeelongdouble is passed through but it does not change -target-abi.
+// RUN: %clang -target powerpc64le-linux-gnu %s -mabi=ieeelongdouble -mabi=elfv1 -### 2>&1 \
+// RUN:   | FileCheck -check-prefix=CHECK-ELFv1-IEEE %s
+// RUN: %clang -target powerpc64le-linux-gnu %s -mabi=elfv1 -mabi=ieeelongdouble -### 2>&1 \
+// RUN:   | FileCheck -check-prefix=CHECK-ELFv1-IEEE %s
 
+// CHECK-ELFv1-IEEE: "-mabi=ieeelongdouble"
+// CHECK-ELFv1-IEEE: "-target-abi" "elfv1"
+
+// Check -mabi=ibmlongdouble is the default.
+// RUN: %clang -target powerpc64le-linux-gnu %s -### 2>&1 \
+// RUN:   | FileCheck -check-prefix=CHECK-ELFv2-IBM128 %s
+// RUN: %clang -target powerpc64le-linux-gnu %s -mabi=ibmlongdouble -### 2>&1 \
+// RUN:   | FileCheck -check-prefix=CHECK-ELFv2-IBM128 %s
+
+// CHECK-ELFv2-IBM128-NOT: "-mabi=ieeelongdouble"
+// CHECK-ELFv2-IBM128: "-target-abi" "elfv2"
Index: test/CodeGen/ppc64-long-double.cpp
===
--- test/CodeGen/ppc64-long-double.cpp
+++ test/CodeGen/ppc64-long-double.cpp
@@ -3,6 +3,14 @@
 // RUN: %clang_cc1 -triple powerpc64-linux-gnu -emit-llvm -o - %s -mlong-double-64 | \
 // RUN:   FileCheck --check-prefix=FP64 %s
 
+// musl defaults to -mlong-double-64, so -mlong-double-128 is needed to make
+// -mabi=ieeelongdouble effective.
+// RUN: %clang_cc1 -triple powerpc64-linux-musl -emit-llvm -o - %s -mlong-double-128 \
+// RUN:   -mabi=ieeelongdouble | FileCheck --check-prefix=FP128 %s
+// RUN: %clang_cc1 -triple powerpc64-linux-gnu -emit-llvm -o - %s \
+// RUN:   -mabi=ieeelongdouble | FileCheck --check-prefix=FP128 %s
+
+// IBM extended double is the default.
 // RUN: %clang_cc1 -triple powerpc64-linux-gnu -emit-llvm -o - %s | \
 // RUN:   FileCheck --check-prefix=IBM128 %s
 // RUN: %clang_cc1 -triple powerpc64-linux-musl -emit-llvm -o - -mlong-double-128 %s | \
@@ -13,10 +21,13 @@
 
 // FP64: @x = global double {{.*}}, align 8
 // FP64: @size = global i32 8
+// FP128: @x = global fp128 {{.*}}, align 16
+// FP128: @size = global i32 16
 // IBM128: @x = global ppc_fp128 {{.*}}, align 16
 // IBM128: @size = global i32 16
 
 long double foo(long double d) { return d; }
 
 // FP64: double @_Z3fooe(double %d)
+// FP128: fp128 @_Z3fooU10__float128(fp128 %d)
 // IBM128: ppc_fp128 @_Z3foog(ppc_fp128 %d)
Index: lib/Frontend/CompilerInvocation.cpp
===
--- lib/Frontend/CompilerInvocation.cpp
+++ lib/Frontend/CompilerInvocation.cpp
@@ -2745,6 +2745,7 @@
   Opts.LongDoubleSize = Args.hasArg(OPT_mlong_double_128)
 ? 128
 : Args.hasArg(OPT_mlong_double_64) ? 64 : 0;
+  Opts.PPCIEEELongDouble = Args.hasArg(OPT_mabi_EQ_ieeelongdouble);
   Opts.PICLevel = getLastArgIntValue(Args, OPT_pic_level, 0, Diags);
   Opts.ROPI = Args.hasArg(OPT_fropi);
   Opts.RWPI = Args.hasArg(OPT_frwpi);
Index: lib/Driver/ToolChains/Clang.cpp
===
--- lib/Driver/ToolChains/Clang.cpp
+++ lib/Driver/ToolChains/Clang.cpp
@@ -1808,12 +1808,27 @@
   break;
 }
 
-  if (Arg *A = Args.getLastArg(options::OPT_mabi_EQ))
-// The ppc64 linux abis are all "altivec" abis by default. Accept and ignore
-// the option if given as we don't have backend support for any targets
-// that don't use the altivec abi.
-if (StringRef(A->getValue()) != "altivec")
-  ABIName = A->getValue();
+  bool IEEELongDouble = false, SeenLongDouble = false, SeenOther = false;
+  for (Arg *A : Args.filtered_reverse(options::OPT_mabi_EQ,
+  options::OPT_mabi_EQ_ieeelongdouble)) {
+if (A->getOption().matches(options::OPT_mabi_EQ_ieeelongdouble)) {
+  A->claim();
+  if (!SeenLongDouble)
+IEEELongDouble = true;
+  SeenLongDouble = true;
+} else if (S

[PATCH] D64277: [X86][PowerPC] Support -mlong-double-128

[Fangrui Song via Phabricator via cfe-commits]

MaskRay updated this revision to Diff 208266.
MaskRay added a comment.
Herald added a subscriber: jdoerfert.

Small adjustment to PPC.cpp to make -mlong-double-128 -mabi=ieeelongdouble 
simpler


Repository:
  rC Clang

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

https://reviews.llvm.org/D64277

Files:
  include/clang/Driver/Options.td
  lib/Basic/TargetInfo.cpp
  lib/Basic/Targets/PPC.cpp
  lib/Basic/Targets/X86.h
  lib/Driver/ToolChains/Clang.cpp
  lib/Frontend/CompilerInvocation.cpp
  test/CodeGen/ppc64-long-double.cpp
  test/CodeGen/x86-long-double.cpp
  test/Driver/mlong-double-128.c
  test/OpenMP/nvptx_unsupported_type_codegen.cpp

Index: test/OpenMP/nvptx_unsupported_type_codegen.cpp
===
--- test/OpenMP/nvptx_unsupported_type_codegen.cpp
+++ test/OpenMP/nvptx_unsupported_type_codegen.cpp
@@ -76,6 +76,3 @@
   f = 1;
   return f;
 }
-
-// CHECK: define weak void @__omp_offloading_{{.+}}foo{{.+}}_l75([[BIGTYPE:.+]]*
-// CHECK: store [[BIGTYPE]] 0xL3FFF, [[BIGTYPE]]* %
Index: test/Driver/mlong-double-128.c
===
--- /dev/null
+++ test/Driver/mlong-double-128.c
@@ -0,0 +1,11 @@
+// RUN: %clang -target powerpc-linux-musl -c -### %s -mlong-double-128 2>&1 | FileCheck %s
+// RUN: %clang -target powerpc64-pc-freebsd12 -c -### %s -mlong-double-128 2>&1 | FileCheck %s
+// RUN: %clang -target powerpc64le-linux-musl -c -### %s -mlong-double-128 2>&1 | FileCheck %s
+// RUN: %clang -target i686-linux-gnu -c -### %s -mlong-double-128 2>&1 | FileCheck %s
+// RUN: %clang -target x86_64-linux-musl -c -### %s -mlong-double-128 2>&1 | FileCheck %s
+
+// CHECK: "-mlong-double-128"
+
+// RUN: %clang -target aarch64 -c -### %s -mlong-double-128 2>&1 | FileCheck --check-prefix=ERR %s
+
+// ERR: error: unsupported option '-mlong-double-128' for target 'aarch64'
Index: test/CodeGen/x86-long-double.cpp
===
--- test/CodeGen/x86-long-double.cpp
+++ test/CodeGen/x86-long-double.cpp
@@ -16,6 +16,15 @@
 // RUN: %clang_cc1 %s -emit-llvm -o - -triple=x86_64-apple-darwin -mlong-double-64 | \
 // RUN:   FileCheck --check-prefixes=FP64,FP64-X64 %s
 
+// RUN: %clang_cc1 %s -emit-llvm -o - -triple=i686 -mlong-double-128 | \
+// RUN:   FileCheck --check-prefix=FP128 %s
+// RUN: %clang_cc1 %s -emit-llvm -o - -triple=i686-apple-darwin -mlong-double-128 | \
+// RUN:   FileCheck --check-prefix=FP128 %s
+// RUN: %clang_cc1 %s -emit-llvm -o - -triple=x86_64 -mlong-double-128 | \
+// RUN:   FileCheck --check-prefix=FP128 %s
+// RUN: %clang_cc1 %s -emit-llvm -o - -triple=x86_64-apple-darwin -mlong-double-128 | \
+// RUN:   FileCheck --check-prefix=FP128 %s
+
 // Check -malign-double increases the alignment from 4 to 8 on x86-32.
 // RUN: %clang_cc1 %s -emit-llvm -o - -triple=i686 -mlong-double-64 \
 // RUN:   -malign-double | FileCheck --check-prefixes=FP64,FP64-X64 %s
@@ -37,7 +46,11 @@
 // FP64-X64: @x = global double {{.*}}, align 8
 // FP64-X64: @size = global i32 8
 
+// FP128: @x = global fp128 {{.*}}, align 16
+// FP128: @size = global i32 16
+
 long double foo(long double d) { return d; }
 
 // FP64: double @_Z3fooe(double %d)
 // FP80: x86_fp80 @_Z3fooe(x86_fp80 %d)
+// FP128: fp128 @_Z3foog(fp128 %d)
Index: test/CodeGen/ppc64-long-double.cpp
===
--- test/CodeGen/ppc64-long-double.cpp
+++ test/CodeGen/ppc64-long-double.cpp
@@ -2,8 +2,11 @@
 // RUN:   FileCheck --check-prefix=FP64 %s
 // RUN: %clang_cc1 -triple powerpc64-linux-gnu -emit-llvm -o - %s -mlong-double-64 | \
 // RUN:   FileCheck --check-prefix=FP64 %s
+
 // RUN: %clang_cc1 -triple powerpc64-linux-gnu -emit-llvm -o - %s | \
 // RUN:   FileCheck --check-prefix=IBM128 %s
+// RUN: %clang_cc1 -triple powerpc64-linux-musl -emit-llvm -o - -mlong-double-128 %s | \
+// RUN:   FileCheck --check-prefix=IBM128 %s
 
 long double x = 0;
 int size = sizeof(x);
Index: lib/Frontend/CompilerInvocation.cpp
===
--- lib/Frontend/CompilerInvocation.cpp
+++ lib/Frontend/CompilerInvocation.cpp
@@ -2742,7 +2742,9 @@
   Opts.PackStruct = getLastArgIntValue(Args, OPT_fpack_struct_EQ, 0, Diags);
   Opts.MaxTypeAlign = getLastArgIntValue(Args, OPT_fmax_type_align_EQ, 0, Diags);
   Opts.AlignDouble = Args.hasArg(OPT_malign_double);
-  Opts.LongDoubleSize = Args.hasArg(OPT_mlong_double_64) ? 64 : 0;
+  Opts.LongDoubleSize = Args.hasArg(OPT_mlong_double_128)
+? 128
+: Args.hasArg(OPT_mlong_double_64) ? 64 : 0;
   Opts.PICLevel = getLastArgIntValue(Args, OPT_pic_level, 0, Diags);
   Opts.ROPI = Args.hasArg(OPT_fropi);
   Opts.RWPI = Args.hasArg(OPT_frwpi);
Index: lib/Driver/ToolChains/Clang.cpp
===
--- lib/Driver/ToolChains/Clang.c

[PATCH] D64270: [analyzer][NFC] Prepare visitors for different tracking kinds

[Csaba Dabis via Phabricator via cfe-commits]

Charusso accepted this revision.
Charusso added a comment.

Document!!4!44! It is great you have started to limit the notes, thanks!




Comment at: 
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h:110
+  ConditionTracking
+};
+

What about the following?
```
enum class TrackKind {
  Full, // comment what it does
  Condition // comment what it does
};
```

Please consider the following example by unspoken naming conventions:
```
enum class ColoringKind {
  RedColoring,
  BlueColoring
}
```
would be
```
enum class Color { Red, Blue }
```
where each trivial what is does, by name.

Please also note that, the thoroughness not working with redecls, assumptions, 
loop conditions, anything we failed to inline, etc... so it is not really that 
full tracking. But in our capabilities, it is the full what we could do.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64270



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

[PATCH] D64277: [X86][PowerPC] Support -mlong-double-128

[Fangrui Song via Phabricator via cfe-commits]

MaskRay updated this revision to Diff 208260.
MaskRay added a comment.

Add a driver test


Repository:
  rC Clang

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

https://reviews.llvm.org/D64277

Files:
  include/clang/Driver/Options.td
  lib/Basic/TargetInfo.cpp
  lib/Basic/Targets/PPC.cpp
  lib/Basic/Targets/X86.h
  lib/Driver/ToolChains/Clang.cpp
  lib/Frontend/CompilerInvocation.cpp
  test/CodeGen/ppc64-long-double.cpp
  test/CodeGen/x86-long-double.cpp
  test/Driver/mlong-double-128.c

Index: test/Driver/mlong-double-128.c
===
--- /dev/null
+++ test/Driver/mlong-double-128.c
@@ -0,0 +1,11 @@
+// RUN: %clang -target powerpc-linux-musl -c -### %s -mlong-double-128 2>&1 | FileCheck %s
+// RUN: %clang -target powerpc64-pc-freebsd12 -c -### %s -mlong-double-128 2>&1 | FileCheck %s
+// RUN: %clang -target powerpc64le-linux-musl -c -### %s -mlong-double-128 2>&1 | FileCheck %s
+// RUN: %clang -target i686-linux-gnu -c -### %s -mlong-double-128 2>&1 | FileCheck %s
+// RUN: %clang -target x86_64-linux-musl -c -### %s -mlong-double-128 2>&1 | FileCheck %s
+
+// CHECK: "-mlong-double-128"
+
+// RUN: %clang -target aarch64 -c -### %s -mlong-double-128 2>&1 | FileCheck --check-prefix=ERR %s
+
+// ERR: error: unsupported option '-mlong-double-128' for target 'aarch64'
Index: test/CodeGen/x86-long-double.cpp
===
--- test/CodeGen/x86-long-double.cpp
+++ test/CodeGen/x86-long-double.cpp
@@ -16,6 +16,15 @@
 // RUN: %clang_cc1 %s -emit-llvm -o - -triple=x86_64-apple-darwin -mlong-double-64 | \
 // RUN:   FileCheck --check-prefixes=FP64,FP64-X64 %s
 
+// RUN: %clang_cc1 %s -emit-llvm -o - -triple=i686 -mlong-double-128 | \
+// RUN:   FileCheck --check-prefix=FP128 %s
+// RUN: %clang_cc1 %s -emit-llvm -o - -triple=i686-apple-darwin -mlong-double-128 | \
+// RUN:   FileCheck --check-prefix=FP128 %s
+// RUN: %clang_cc1 %s -emit-llvm -o - -triple=x86_64 -mlong-double-128 | \
+// RUN:   FileCheck --check-prefix=FP128 %s
+// RUN: %clang_cc1 %s -emit-llvm -o - -triple=x86_64-apple-darwin -mlong-double-128 | \
+// RUN:   FileCheck --check-prefix=FP128 %s
+
 // Check -malign-double increases the alignment from 4 to 8 on x86-32.
 // RUN: %clang_cc1 %s -emit-llvm -o - -triple=i686 -mlong-double-64 \
 // RUN:   -malign-double | FileCheck --check-prefixes=FP64,FP64-X64 %s
@@ -37,7 +46,11 @@
 // FP64-X64: @x = global double {{.*}}, align 8
 // FP64-X64: @size = global i32 8
 
+// FP128: @x = global fp128 {{.*}}, align 16
+// FP128: @size = global i32 16
+
 long double foo(long double d) { return d; }
 
 // FP64: double @_Z3fooe(double %d)
 // FP80: x86_fp80 @_Z3fooe(x86_fp80 %d)
+// FP128: fp128 @_Z3foog(fp128 %d)
Index: test/CodeGen/ppc64-long-double.cpp
===
--- test/CodeGen/ppc64-long-double.cpp
+++ test/CodeGen/ppc64-long-double.cpp
@@ -2,8 +2,11 @@
 // RUN:   FileCheck --check-prefix=FP64 %s
 // RUN: %clang_cc1 -triple powerpc64-linux-gnu -emit-llvm -o - %s -mlong-double-64 | \
 // RUN:   FileCheck --check-prefix=FP64 %s
+
 // RUN: %clang_cc1 -triple powerpc64-linux-gnu -emit-llvm -o - %s | \
 // RUN:   FileCheck --check-prefix=IBM128 %s
+// RUN: %clang_cc1 -triple powerpc64-linux-musl -emit-llvm -o - -mlong-double-128 %s | \
+// RUN:   FileCheck --check-prefix=IBM128 %s
 
 long double x = 0;
 int size = sizeof(x);
Index: lib/Frontend/CompilerInvocation.cpp
===
--- lib/Frontend/CompilerInvocation.cpp
+++ lib/Frontend/CompilerInvocation.cpp
@@ -2742,7 +2742,9 @@
   Opts.PackStruct = getLastArgIntValue(Args, OPT_fpack_struct_EQ, 0, Diags);
   Opts.MaxTypeAlign = getLastArgIntValue(Args, OPT_fmax_type_align_EQ, 0, Diags);
   Opts.AlignDouble = Args.hasArg(OPT_malign_double);
-  Opts.LongDoubleSize = Args.hasArg(OPT_mlong_double_64) ? 64 : 0;
+  Opts.LongDoubleSize = Args.hasArg(OPT_mlong_double_128)
+? 128
+: Args.hasArg(OPT_mlong_double_64) ? 64 : 0;
   Opts.PICLevel = getLastArgIntValue(Args, OPT_pic_level, 0, Diags);
   Opts.ROPI = Args.hasArg(OPT_fropi);
   Opts.RWPI = Args.hasArg(OPT_frwpi);
Index: lib/Driver/ToolChains/Clang.cpp
===
--- lib/Driver/ToolChains/Clang.cpp
+++ lib/Driver/ToolChains/Clang.cpp
@@ -4001,11 +4001,14 @@
 
   RenderFloatingPointOptions(TC, D, OFastEnabled, Args, CmdArgs);
 
-  if (Arg *A = Args.getLastArg(options::OPT_mlong_double_64)) {
+  if (Arg *A = Args.getLastArg(options::OPT_mlong_double_64,
+   options::OPT_mlong_double_128)) {
 if (TC.getArch() == llvm::Triple::x86 ||
 TC.getArch() == llvm::Triple::x86_64 ||
 TC.getArch() == llvm::Triple::ppc || TC.getTriple().isPPC64()) {
-  CmdArgs.push_back("-mlong-double-64");
+  CmdArgs.push_back(A->getOption().matc

[PATCH] D64278: Rename libclang_shared to libclang-cpp

[Sylvestre Ledru via Phabricator via cfe-commits]

sylvestre.ledru created this revision.
sylvestre.ledru added reviewers: beanz, tstellar.
Herald added subscribers: cfe-commits, mgorny.
Herald added a project: clang.
sylvestre.ledru edited the summary of this revision.

Fix bug 42475


Repository:
  rC Clang

https://reviews.llvm.org/D64278

Files:
  CMakeLists.txt
  cmake/modules/AddClang.cmake
  tools/clang-shlib/CMakeLists.txt


Index: tools/clang-shlib/CMakeLists.txt
===
--- tools/clang-shlib/CMakeLists.txt
+++ tools/clang-shlib/CMakeLists.txt
@@ -1,4 +1,4 @@
-# Building libclang_shared.so fails if LLVM_ENABLE_PIC=Off
+# Building libclang-cpp.so fails if LLVM_ENABLE_PIC=Off
 if (NOT LLVM_ENABLE_PIC)
   return()
 endif()
@@ -11,7 +11,7 @@
   list(APPEND _DEPS $)
 endforeach ()
 
-add_clang_library(clang_shared
+add_clang_library(clang-cpp
   SHARED
   clang-shlib.cpp
   ${_OBJECTS}
Index: cmake/modules/AddClang.cmake
===
--- cmake/modules/AddClang.cmake
+++ cmake/modules/AddClang.cmake
@@ -175,7 +175,7 @@
 
 function(clang_target_link_libraries target type)
   if (CLANG_LINK_CLANG_DYLIB)
-target_link_libraries(${target} ${type} clang_shared)
+target_link_libraries(${target} ${type} clang-cpp)
   else()
 target_link_libraries(${target} ${type} ${ARGN})
   endif()
Index: CMakeLists.txt
===
--- CMakeLists.txt
+++ CMakeLists.txt
@@ -324,7 +324,7 @@
 "Python versions to install libclang python bindings for")
 
 set(CLANG_LINK_CLANG_DYLIB ${LLVM_LINK_LLVM_DYLIB} CACHE BOOL
-"Link tools against libclang_shared.so")
+"Link tools against libclang-cpp.so")
 
 if (NOT LLVM_LINK_LLVM_DYLIB AND CLANG_LINK_CLANG_DYLIB)
   message(FATAL_ERROR "Cannot set CLANG_LINK_CLANG_DYLIB=ON when "


Index: tools/clang-shlib/CMakeLists.txt
===
--- tools/clang-shlib/CMakeLists.txt
+++ tools/clang-shlib/CMakeLists.txt
@@ -1,4 +1,4 @@
-# Building libclang_shared.so fails if LLVM_ENABLE_PIC=Off
+# Building libclang-cpp.so fails if LLVM_ENABLE_PIC=Off
 if (NOT LLVM_ENABLE_PIC)
   return()
 endif()
@@ -11,7 +11,7 @@
   list(APPEND _DEPS $)
 endforeach ()
 
-add_clang_library(clang_shared
+add_clang_library(clang-cpp
   SHARED
   clang-shlib.cpp
   ${_OBJECTS}
Index: cmake/modules/AddClang.cmake
===
--- cmake/modules/AddClang.cmake
+++ cmake/modules/AddClang.cmake
@@ -175,7 +175,7 @@
 
 function(clang_target_link_libraries target type)
   if (CLANG_LINK_CLANG_DYLIB)
-target_link_libraries(${target} ${type} clang_shared)
+target_link_libraries(${target} ${type} clang-cpp)
   else()
 target_link_libraries(${target} ${type} ${ARGN})
   endif()
Index: CMakeLists.txt
===
--- CMakeLists.txt
+++ CMakeLists.txt
@@ -324,7 +324,7 @@
 "Python versions to install libclang python bindings for")
 
 set(CLANG_LINK_CLANG_DYLIB ${LLVM_LINK_LLVM_DYLIB} CACHE BOOL
-"Link tools against libclang_shared.so")
+"Link tools against libclang-cpp.so")
 
 if (NOT LLVM_LINK_LLVM_DYLIB AND CLANG_LINK_CLANG_DYLIB)
   message(FATAL_ERROR "Cannot set CLANG_LINK_CLANG_DYLIB=ON when "
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D64277: [X86][PowerPC] Support -mlong-double-128

[Fangrui Song via Phabricator via cfe-commits]

MaskRay created this revision.
MaskRay added reviewers: awilfox, echristo, erichkeane, hfinkel, jsji, 
majnemer, rnk, rsmith.
Herald added subscribers: cfe-commits, kbarton, nemanjai.
Herald added a project: clang.

This patch makes the driver option -mlong-double-128 available for X86
and PowerPC. The CC1 option -mlong-double-128 is available on all targets
for users to test on unsupported targets.

On PowerPC, -mlong-double-128 uses the IBM extended double format
because we don't support -mabi=ieeelongdouble.


Repository:
  rC Clang

https://reviews.llvm.org/D64277

Files:
  include/clang/Driver/Options.td
  lib/Basic/TargetInfo.cpp
  lib/Basic/Targets/PPC.cpp
  lib/Basic/Targets/X86.h
  lib/Driver/ToolChains/Clang.cpp
  lib/Frontend/CompilerInvocation.cpp
  test/CodeGen/ppc64-long-double.cpp
  test/CodeGen/x86-long-double.cpp

Index: test/CodeGen/x86-long-double.cpp
===
--- test/CodeGen/x86-long-double.cpp
+++ test/CodeGen/x86-long-double.cpp
@@ -16,6 +16,15 @@
 // RUN: %clang_cc1 %s -emit-llvm -o - -triple=x86_64-apple-darwin -mlong-double-64 | \
 // RUN:   FileCheck --check-prefixes=FP64,FP64-X64 %s
 
+// RUN: %clang_cc1 %s -emit-llvm -o - -triple=i686 -mlong-double-128 | \
+// RUN:   FileCheck --check-prefix=FP128 %s
+// RUN: %clang_cc1 %s -emit-llvm -o - -triple=i686-apple-darwin -mlong-double-128 | \
+// RUN:   FileCheck --check-prefix=FP128 %s
+// RUN: %clang_cc1 %s -emit-llvm -o - -triple=x86_64 -mlong-double-128 | \
+// RUN:   FileCheck --check-prefix=FP128 %s
+// RUN: %clang_cc1 %s -emit-llvm -o - -triple=x86_64-apple-darwin -mlong-double-128 | \
+// RUN:   FileCheck --check-prefix=FP128 %s
+
 // Check -malign-double increases the alignment from 4 to 8 on x86-32.
 // RUN: %clang_cc1 %s -emit-llvm -o - -triple=i686 -mlong-double-64 \
 // RUN:   -malign-double | FileCheck --check-prefixes=FP64,FP64-X64 %s
@@ -37,7 +46,11 @@
 // FP64-X64: @x = global double {{.*}}, align 8
 // FP64-X64: @size = global i32 8
 
+// FP128: @x = global fp128 {{.*}}, align 16
+// FP128: @size = global i32 16
+
 long double foo(long double d) { return d; }
 
 // FP64: double @_Z3fooe(double %d)
 // FP80: x86_fp80 @_Z3fooe(x86_fp80 %d)
+// FP128: fp128 @_Z3foog(fp128 %d)
Index: test/CodeGen/ppc64-long-double.cpp
===
--- test/CodeGen/ppc64-long-double.cpp
+++ test/CodeGen/ppc64-long-double.cpp
@@ -2,8 +2,11 @@
 // RUN:   FileCheck --check-prefix=FP64 %s
 // RUN: %clang_cc1 -triple powerpc64-linux-gnu -emit-llvm -o - %s -mlong-double-64 | \
 // RUN:   FileCheck --check-prefix=FP64 %s
+
 // RUN: %clang_cc1 -triple powerpc64-linux-gnu -emit-llvm -o - %s | \
 // RUN:   FileCheck --check-prefix=IBM128 %s
+// RUN: %clang_cc1 -triple powerpc64-linux-musl -emit-llvm -o - -mlong-double-128 %s | \
+// RUN:   FileCheck --check-prefix=IBM128 %s
 
 long double x = 0;
 int size = sizeof(x);
Index: lib/Frontend/CompilerInvocation.cpp
===
--- lib/Frontend/CompilerInvocation.cpp
+++ lib/Frontend/CompilerInvocation.cpp
@@ -2742,7 +2742,9 @@
   Opts.PackStruct = getLastArgIntValue(Args, OPT_fpack_struct_EQ, 0, Diags);
   Opts.MaxTypeAlign = getLastArgIntValue(Args, OPT_fmax_type_align_EQ, 0, Diags);
   Opts.AlignDouble = Args.hasArg(OPT_malign_double);
-  Opts.LongDoubleSize = Args.hasArg(OPT_mlong_double_64) ? 64 : 0;
+  Opts.LongDoubleSize = Args.hasArg(OPT_mlong_double_128)
+? 128
+: Args.hasArg(OPT_mlong_double_64) ? 64 : 0;
   Opts.PICLevel = getLastArgIntValue(Args, OPT_pic_level, 0, Diags);
   Opts.ROPI = Args.hasArg(OPT_fropi);
   Opts.RWPI = Args.hasArg(OPT_frwpi);
Index: lib/Driver/ToolChains/Clang.cpp
===
--- lib/Driver/ToolChains/Clang.cpp
+++ lib/Driver/ToolChains/Clang.cpp
@@ -4001,11 +4001,14 @@
 
   RenderFloatingPointOptions(TC, D, OFastEnabled, Args, CmdArgs);
 
-  if (Arg *A = Args.getLastArg(options::OPT_mlong_double_64)) {
+  if (Arg *A = Args.getLastArg(options::OPT_mlong_double_64,
+   options::OPT_mlong_double_128)) {
 if (TC.getArch() == llvm::Triple::x86 ||
 TC.getArch() == llvm::Triple::x86_64 ||
 TC.getArch() == llvm::Triple::ppc || TC.getTriple().isPPC64()) {
-  CmdArgs.push_back("-mlong-double-64");
+  CmdArgs.push_back(A->getOption().matches(options::OPT_mlong_double_64)
+? "-mlong-double-64"
+: "-mlong-double-128");
 } else {
   D.Diag(diag::err_drv_unsupported_opt_for_target)
   << A->getAsString(Args) << TripleStr;
Index: lib/Basic/Targets/X86.h
===
--- lib/Basic/Targets/X86.h
+++ lib/Basic/Targets/X86.h
@@ -133,6 +133,13 @@
 LongDoubleFormat = &llvm::APFloat::x87DoubleExtended();
   }
 
+  const char *getMa

[PATCH] D64276: [ItaniumMangle] Replace useFloat128ManglingForLongDouble() with getManglingForLongDouble() and getManglingForFloat128()

[Fangrui Song via Phabricator via cfe-commits]

MaskRay updated this revision to Diff 208256.
MaskRay added a comment.

Simplify x86_64 Android. Since it doesn't use -mlong-double-64 or 
-mlong-double-128, just use "g" for it.
In the next -mlong-double-128 patch, we move its getManglingForLongDouble() 
overload to X86TargetInfo


Repository:
  rC Clang

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

https://reviews.llvm.org/D64276

Files:
  include/clang/Basic/TargetInfo.h
  lib/AST/ItaniumMangle.cpp
  lib/Basic/Targets/PPC.h
  lib/Basic/Targets/SystemZ.h
  lib/Basic/Targets/X86.h

Index: lib/Basic/Targets/X86.h
===
--- lib/Basic/Targets/X86.h
+++ lib/Basic/Targets/X86.h
@@ -848,7 +848,7 @@
 LongDoubleFormat = &llvm::APFloat::IEEEquad();
   }
 
-  bool useFloat128ManglingForLongDouble() const override { return true; }
+  const char *getManglingForLongDouble() const override { return "g"; }
 };
 } // namespace targets
 } // namespace clang
Index: lib/Basic/Targets/SystemZ.h
===
--- lib/Basic/Targets/SystemZ.h
+++ lib/Basic/Targets/SystemZ.h
@@ -141,7 +141,7 @@
 return "";
   }
 
-  bool useFloat128ManglingForLongDouble() const override { return true; }
+  const char *getManglingForLongDouble() const override { return "g"; }
 };
 } // namespace targets
 } // namespace clang
Index: lib/Basic/Targets/PPC.h
===
--- lib/Basic/Targets/PPC.h
+++ lib/Basic/Targets/PPC.h
@@ -314,10 +314,15 @@
 
   bool hasSjLjLowering() const override { return true; }
 
-  bool useFloat128ManglingForLongDouble() const override {
-return LongDoubleWidth == 128 &&
-   LongDoubleFormat == &llvm::APFloat::PPCDoubleDouble() &&
-   getTriple().isOSBinFormatELF();
+  const char *getManglingForLongDouble() const override {
+if (LongDoubleWidth == 64)
+  return "e";
+return LongDoubleFormat == &llvm::APFloat::PPCDoubleDouble()
+   ? "g"
+   : "U10__float128";
+  }
+  const char *getManglingForFloat128() const override {
+return "U10__float128";
   }
 };
 
Index: lib/AST/ItaniumMangle.cpp
===
--- lib/AST/ItaniumMangle.cpp
+++ lib/AST/ItaniumMangle.cpp
@@ -2608,30 +2608,19 @@
 Out << 'd';
 break;
   case BuiltinType::LongDouble: {
-bool UseFloat128Mangling =
-getASTContext().getTargetInfo().useFloat128ManglingForLongDouble();
-if (getASTContext().getLangOpts().OpenMP &&
-getASTContext().getLangOpts().OpenMPIsDevice) {
-  UseFloat128Mangling = getASTContext()
-.getAuxTargetInfo()
-->useFloat128ManglingForLongDouble();
-}
-Out << (UseFloat128Mangling ? 'g' : 'e');
+const TargetInfo *TI = getASTContext().getLangOpts().OpenMP &&
+   getASTContext().getLangOpts().OpenMPIsDevice
+   ? getASTContext().getAuxTargetInfo()
+   : &getASTContext().getTargetInfo();
+Out << TI->getManglingForLongDouble();
 break;
   }
   case BuiltinType::Float128: {
-bool UseFloat128Mangling =
-getASTContext().getTargetInfo().useFloat128ManglingForLongDouble();
-if (getASTContext().getLangOpts().OpenMP &&
-getASTContext().getLangOpts().OpenMPIsDevice) {
-  UseFloat128Mangling = getASTContext()
-.getAuxTargetInfo()
-->useFloat128ManglingForLongDouble();
-}
-if (UseFloat128Mangling)
-  Out << "U10__float128"; // Match the GCC mangling
-else
-  Out << 'g';
+const TargetInfo *TI = getASTContext().getLangOpts().OpenMP &&
+   getASTContext().getLangOpts().OpenMPIsDevice
+   ? getASTContext().getAuxTargetInfo()
+   : &getASTContext().getTargetInfo();
+Out << TI->getManglingForFloat128();
 break;
   }
   case BuiltinType::NullPtr:
Index: include/clang/Basic/TargetInfo.h
===
--- include/clang/Basic/TargetInfo.h
+++ include/clang/Basic/TargetInfo.h
@@ -599,9 +599,11 @@
 return *Float128Format;
   }
 
-  /// Return true if the 'long double' type should be mangled like
-  /// __float128.
-  virtual bool useFloat128ManglingForLongDouble() const { return false; }
+  /// Return the mangled type of long double.
+  virtual const char *getManglingForLongDouble() const { return "e"; }
+
+  /// Return the mangled type of __float128.
+  virtual const char *getManglingForFloat128() const { return "g"; }
 
   /// Return the value for the C99 FLT_EVAL_METHOD macro.
   virtual unsigned getFloatEvalMethod() const { return 0; }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https