[PATCH] D45716: [XRay] Add clang builtin for xray typed events.
kpw created this revision.
kpw added reviewers: dberris, pelikan, rnk, eizan.
A clang builtin for xray typed events. Differs from
__xray_customevent(...) by the presence of a type tag that is vended by
compiler-rt in typical usage. This allows xray handlers to expand logged
events with their type description and plugins to process traced events
based on type.
This change depends on https://reviews.llvm.org/D45633 for the intrinsic
definition.
Repository:
rC Clang
https://reviews.llvm.org/D45716
Files:
include/clang/Basic/Builtins.def
lib/CodeGen/CGBuiltin.cpp
test/CodeGen/xray-typedevent.cpp
Index: test/CodeGen/xray-typedevent.cpp
===
--- /dev/null
+++ test/CodeGen/xray-typedevent.cpp
@@ -0,0 +1,32 @@
+// RUN: %clang_cc1 -fxray-instrument -x c++ -std=c++11 -triple
x86_64-unknown-unknown -emit-llvm -o - %s | FileCheck %s
+
+// CHECK-LABEL: @_Z16alwaysInstrumentv
+[[clang::xray_always_instrument]] void alwaysInstrument() {
+ auto EventType = 1;
+ static constexpr char kPhase[] = "instrument";
+ __xray_typedevent(EventType, kPhase, 10);
+ // CHECK: call void @llvm.xray.typedevent(i16 {{.*}}, i8*{{.*}}, i32 10)
+}
+
+// CHECK-LABEL: @_Z15neverInstrumentv
+[[clang::xray_never_instrument]] void neverInstrument() {
+ auto EventType = 2;
+ static constexpr char kPhase[] = "never";
+ __xray_typedevent(EventType, kPhase, 5);
+ // CHECK-NOT: call void @llvm.xray.typedevent(i16 {{.*}}, i8*{{.*}}, i32 5)
+}
+
+// CHECK-LABEL: @_Z21conditionalInstrumenti
+[[clang::xray_always_instrument]] void conditionalInstrument(int v) {
+ auto TrueEventType = 3;
+ auto UntrueEventType = 4;
+ static constexpr char kTrue[] = "true";
+ static constexpr char kUntrue[] = "untrue";
+ if (v % 2)
+__xray_typedevent(TrueEventType, kTrue, 4);
+ else
+__xray_typedevent(UntrueEventType, kUntrue, 6);
+
+ // CHECK: call void @llvm.xray.typedevent(i16 {{.*}}, i8*{{.*}}, i32 4)
+ // CHECK: call void @llvm.xray.typedevent(i16 {{.*}}, i8*{{.*}}, i32 6)
+}
Index: lib/CodeGen/CGBuiltin.cpp
===
--- lib/CodeGen/CGBuiltin.cpp
+++ lib/CodeGen/CGBuiltin.cpp
@@ -3369,6 +3369,41 @@
return RValue::get(Builder.CreateCall(F, {Arg0Val, Arg1}));
}
+ case Builtin::BI__xray_typedevent: {
+if (!ShouldXRayInstrumentFunction())
+ return RValue::getIgnored();
+
+if (!CGM.getCodeGenOpts().XRayInstrumentationBundle.has(
+XRayInstrKind::Custom))
+ return RValue::getIgnored();
+
+if (const auto *XRayAttr = CurFuncDecl->getAttr())
+ if (XRayAttr->neverXRayInstrument() && !AlwaysEmitXRayCustomEvents())
+return RValue::getIgnored();
+
+Function *F = CGM.getIntrinsic(Intrinsic::xray_typedevent);
+auto FTy = F->getFunctionType();
+auto Arg0 = EmitScalarExpr(E->getArg(0));
+auto PTy0 = FTy->getParamType(0);
+if (PTy0 != Arg0->getType())
+ Arg0 = Builder.CreateTruncOrBitCast(Arg0, PTy0);
+auto Arg1 = E->getArg(1);
+auto Arg1Val = EmitScalarExpr(Arg1);
+auto Arg1Ty = Arg1->getType();
+auto PTy1 = FTy->getParamType(1);
+if (PTy1 != Arg1Val->getType()) {
+ if (Arg1Ty->isArrayType())
+Arg1Val = EmitArrayToPointerDecay(Arg1).getPointer();
+ else
+Arg1Val = Builder.CreatePointerCast(Arg1Val, PTy1);
+}
+auto Arg2 = EmitScalarExpr(E->getArg(2));
+auto PTy2 = FTy->getParamType(2);
+if (PTy2 != Arg2->getType())
+ Arg2 = Builder.CreateTruncOrBitCast(Arg2, PTy2);
+return RValue::get(Builder.CreateCall(F, {Arg0, Arg1Val, Arg2}));
+ }
+
case Builtin::BI__builtin_ms_va_start:
case Builtin::BI__builtin_ms_va_end:
return RValue::get(
Index: include/clang/Basic/Builtins.def
===
--- include/clang/Basic/Builtins.def
+++ include/clang/Basic/Builtins.def
@@ -1455,6 +1455,7 @@
// Builtins for XRay
BUILTIN(__xray_customevent, "vcC*z", "")
+BUILTIN(__xray_typedevent, "vzcC*z", "")
// Win64-compatible va_list functions
BUILTIN(__builtin_ms_va_start, "vc*&.", "nt")
Index: test/CodeGen/xray-typedevent.cpp
===
--- /dev/null
+++ test/CodeGen/xray-typedevent.cpp
@@ -0,0 +1,32 @@
+// RUN: %clang_cc1 -fxray-instrument -x c++ -std=c++11 -triple x86_64-unknown-unknown -emit-llvm -o - %s | FileCheck %s
+
+// CHECK-LABEL: @_Z16alwaysInstrumentv
+[[clang::xray_always_instrument]] void alwaysInstrument() {
+ auto EventType = 1;
+ static constexpr char kPhase[] = "instrument";
+ __xray_typedevent(EventType, kPhase, 10);
+ // CHECK: call void @llvm.xray.typedevent(i16 {{.*}}, i8*{{.*}}, i32 10)
+}
+
+// CHECK-LABEL: @_Z15neverInstrumentv
+[[clang::xray_never_instrument]] void neverInstrument() {
+ auto EventType = 2;
+ static constexpr char kPhase[] = "never";
+ __xray_typedevent(EventType, kPhase, 5);
+ // CHECK-NOT: call void @llvm.xray.typedeven
[PATCH] D45716: [XRay] Add clang builtin for xray typed events.
kpw updated this revision to Diff 142735.
kpw added a comment.
Adding a comment to the test to encourage getting the event types from
compiler-rt
Repository:
rC Clang
https://reviews.llvm.org/D45716
Files:
include/clang/Basic/Builtins.def
lib/CodeGen/CGBuiltin.cpp
test/CodeGen/xray-typedevent.cpp
Index: test/CodeGen/xray-typedevent.cpp
===
--- /dev/null
+++ test/CodeGen/xray-typedevent.cpp
@@ -0,0 +1,34 @@
+// RUN: %clang_cc1 -fxray-instrument -x c++ -std=c++11 -triple
x86_64-unknown-unknown -emit-llvm -o - %s | FileCheck %s
+
+// CHECK-LABEL: @_Z16alwaysInstrumentv
+[[clang::xray_always_instrument]] void alwaysInstrument() {
+ // Event types would normally come from calling __xray_register_event_type
+ // from compiler-rt
+ auto EventType = 1;
+ static constexpr char kPhase[] = "instrument";
+ __xray_typedevent(EventType, kPhase, 10);
+ // CHECK: call void @llvm.xray.typedevent(i16 {{.*}}, i8*{{.*}}, i32 10)
+}
+
+// CHECK-LABEL: @_Z15neverInstrumentv
+[[clang::xray_never_instrument]] void neverInstrument() {
+ auto EventType = 2;
+ static constexpr char kPhase[] = "never";
+ __xray_typedevent(EventType, kPhase, 5);
+ // CHECK-NOT: call void @llvm.xray.typedevent(i16 {{.*}}, i8*{{.*}}, i32 5)
+}
+
+// CHECK-LABEL: @_Z21conditionalInstrumenti
+[[clang::xray_always_instrument]] void conditionalInstrument(int v) {
+ auto TrueEventType = 3;
+ auto UntrueEventType = 4;
+ static constexpr char kTrue[] = "true";
+ static constexpr char kUntrue[] = "untrue";
+ if (v % 2)
+__xray_typedevent(TrueEventType, kTrue, 4);
+ else
+__xray_typedevent(UntrueEventType, kUntrue, 6);
+
+ // CHECK: call void @llvm.xray.typedevent(i16 {{.*}}, i8*{{.*}}, i32 4)
+ // CHECK: call void @llvm.xray.typedevent(i16 {{.*}}, i8*{{.*}}, i32 6)
+}
Index: lib/CodeGen/CGBuiltin.cpp
===
--- lib/CodeGen/CGBuiltin.cpp
+++ lib/CodeGen/CGBuiltin.cpp
@@ -3369,6 +3369,41 @@
return RValue::get(Builder.CreateCall(F, {Arg0Val, Arg1}));
}
+ case Builtin::BI__xray_typedevent: {
+if (!ShouldXRayInstrumentFunction())
+ return RValue::getIgnored();
+
+if (!CGM.getCodeGenOpts().XRayInstrumentationBundle.has(
+XRayInstrKind::Custom))
+ return RValue::getIgnored();
+
+if (const auto *XRayAttr = CurFuncDecl->getAttr())
+ if (XRayAttr->neverXRayInstrument() && !AlwaysEmitXRayCustomEvents())
+return RValue::getIgnored();
+
+Function *F = CGM.getIntrinsic(Intrinsic::xray_typedevent);
+auto FTy = F->getFunctionType();
+auto Arg0 = EmitScalarExpr(E->getArg(0));
+auto PTy0 = FTy->getParamType(0);
+if (PTy0 != Arg0->getType())
+ Arg0 = Builder.CreateTruncOrBitCast(Arg0, PTy0);
+auto Arg1 = E->getArg(1);
+auto Arg1Val = EmitScalarExpr(Arg1);
+auto Arg1Ty = Arg1->getType();
+auto PTy1 = FTy->getParamType(1);
+if (PTy1 != Arg1Val->getType()) {
+ if (Arg1Ty->isArrayType())
+Arg1Val = EmitArrayToPointerDecay(Arg1).getPointer();
+ else
+Arg1Val = Builder.CreatePointerCast(Arg1Val, PTy1);
+}
+auto Arg2 = EmitScalarExpr(E->getArg(2));
+auto PTy2 = FTy->getParamType(2);
+if (PTy2 != Arg2->getType())
+ Arg2 = Builder.CreateTruncOrBitCast(Arg2, PTy2);
+return RValue::get(Builder.CreateCall(F, {Arg0, Arg1Val, Arg2}));
+ }
+
case Builtin::BI__builtin_ms_va_start:
case Builtin::BI__builtin_ms_va_end:
return RValue::get(
Index: include/clang/Basic/Builtins.def
===
--- include/clang/Basic/Builtins.def
+++ include/clang/Basic/Builtins.def
@@ -1455,6 +1455,7 @@
// Builtins for XRay
BUILTIN(__xray_customevent, "vcC*z", "")
+BUILTIN(__xray_typedevent, "vzcC*z", "")
// Win64-compatible va_list functions
BUILTIN(__builtin_ms_va_start, "vc*&.", "nt")
Index: test/CodeGen/xray-typedevent.cpp
===
--- /dev/null
+++ test/CodeGen/xray-typedevent.cpp
@@ -0,0 +1,34 @@
+// RUN: %clang_cc1 -fxray-instrument -x c++ -std=c++11 -triple x86_64-unknown-unknown -emit-llvm -o - %s | FileCheck %s
+
+// CHECK-LABEL: @_Z16alwaysInstrumentv
+[[clang::xray_always_instrument]] void alwaysInstrument() {
+ // Event types would normally come from calling __xray_register_event_type
+ // from compiler-rt
+ auto EventType = 1;
+ static constexpr char kPhase[] = "instrument";
+ __xray_typedevent(EventType, kPhase, 10);
+ // CHECK: call void @llvm.xray.typedevent(i16 {{.*}}, i8*{{.*}}, i32 10)
+}
+
+// CHECK-LABEL: @_Z15neverInstrumentv
+[[clang::xray_never_instrument]] void neverInstrument() {
+ auto EventType = 2;
+ static constexpr char kPhase[] = "never";
+ __xray_typedevent(EventType, kPhase, 5);
+ // CHECK-NOT: call void @llvm.xray.typedevent(i16 {{.*}}, i8*{{.*}}, i32 5)
+}
+
+// CHECK-LABEL: @_Z21conditionalInstrumenti
+[[clang::xray_alwa
[PATCH] D45716: [XRay] Add clang builtin for xray typed events.
kpw added inline comments.
Comment at: test/CodeGen/xray-typedevent.cpp:10
+ __xray_typedevent(EventType, kPhase, 10);
+ // CHECK: call void @llvm.xray.typedevent(i16 {{.*}}, i8*{{.*}}, i32 10)
+}
FYI: It would be involved to match on more than * for the event type because
the actual IR does things like unsigned extension and truncation to i16. The IR
looked sane to me though. It's a shame the BuiltIns.def type attributes are
defined in terms like short, half, and size_t rather than fixed width types.
/shrug
Repository:
rC Clang
https://reviews.llvm.org/D45716
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45716: [XRay] Add clang builtin for xray typed events.
kpw updated this revision to Diff 142737.
kpw added a comment.
Added flags and bundle options.
Repository:
rC Clang
https://reviews.llvm.org/D45716
Files:
include/clang/Basic/Builtins.def
include/clang/Basic/LangOptions.def
include/clang/Basic/XRayInstr.h
include/clang/Driver/Options.td
include/clang/Driver/XRayArgs.h
include/clang/Frontend/CodeGenOptions.def
lib/Basic/XRayInstr.cpp
lib/CodeGen/CGBuiltin.cpp
lib/CodeGen/CodeGenFunction.cpp
lib/CodeGen/CodeGenFunction.h
lib/Driver/XRayArgs.cpp
lib/Frontend/CompilerInvocation.cpp
test/CodeGen/xray-always-emit-typedevent.cpp
test/CodeGen/xray-instrumentation-bundles.cpp
test/CodeGen/xray-typedevent.cpp
Index: test/CodeGen/xray-typedevent.cpp
===
--- /dev/null
+++ test/CodeGen/xray-typedevent.cpp
@@ -0,0 +1,34 @@
+// RUN: %clang_cc1 -fxray-instrument -x c++ -std=c++11 -triple x86_64-unknown-unknown -emit-llvm -o - %s | FileCheck %s
+
+// CHECK-LABEL: @_Z16alwaysInstrumentv
+[[clang::xray_always_instrument]] void alwaysInstrument() {
+ // Event types would normally come from calling __xray_register_event_type
+ // from compiler-rt
+ auto EventType = 1;
+ static constexpr char kPhase[] = "instrument";
+ __xray_typedevent(EventType, kPhase, 10);
+ // CHECK: call void @llvm.xray.typedevent(i16 {{.*}}, i8*{{.*}}, i32 10)
+}
+
+// CHECK-LABEL: @_Z15neverInstrumentv
+[[clang::xray_never_instrument]] void neverInstrument() {
+ auto EventType = 2;
+ static constexpr char kPhase[] = "never";
+ __xray_typedevent(EventType, kPhase, 5);
+ // CHECK-NOT: call void @llvm.xray.typedevent(i16 {{.*}}, i8*{{.*}}, i32 5)
+}
+
+// CHECK-LABEL: @_Z21conditionalInstrumenti
+[[clang::xray_always_instrument]] void conditionalInstrument(int v) {
+ auto TrueEventType = 3;
+ auto UntrueEventType = 4;
+ static constexpr char kTrue[] = "true";
+ static constexpr char kUntrue[] = "untrue";
+ if (v % 2)
+__xray_typedevent(TrueEventType, kTrue, 4);
+ else
+__xray_typedevent(UntrueEventType, kUntrue, 6);
+
+ // CHECK: call void @llvm.xray.typedevent(i16 {{.*}}, i8*{{.*}}, i32 4)
+ // CHECK: call void @llvm.xray.typedevent(i16 {{.*}}, i8*{{.*}}, i32 6)
+}
Index: test/CodeGen/xray-instrumentation-bundles.cpp
===
--- test/CodeGen/xray-instrumentation-bundles.cpp
+++ test/CodeGen/xray-instrumentation-bundles.cpp
@@ -1,30 +1,49 @@
// RUN: %clang_cc1 -fxray-instrument -fxray-instrumentation-bundle=none -x c++ \
// RUN: -std=c++11 -triple x86_64-unknown-unknown -emit-llvm -o - %s \
-// RUN: | FileCheck --check-prefixes CHECK,NOFUNCTION,NOCUSTOM %s
-// RUN: %clang_cc1 -fxray-instrument \
-// RUN: -fxray-instrumentation-bundle=function -x c++ \
+// RUN: | FileCheck --check-prefixes CHECK,NOFUNCTION,NOCUSTOM,NOTYPED %s
+// RUN: %clang_cc1 -fxray-instrument -fxray-instrumentation-bundle=function -x c++ \
// RUN: -std=c++11 -triple x86_64-unknown-unknown -emit-llvm -o - %s \
-// RUN: | FileCheck --check-prefixes CHECK,FUNCTION,NOCUSTOM %s
+// RUN: | FileCheck --check-prefixes CHECK,FUNCTION,NOCUSTOM,NOTYPED %s
// RUN: %clang_cc1 -fxray-instrument \
// RUN: -fxray-instrumentation-bundle=custom -x c++ \
// RUN: -std=c++11 -triple x86_64-unknown-unknown -emit-llvm -o - %s \
-// RUN: | FileCheck --check-prefixes CHECK,NOFUNCTION,CUSTOM %s
+// RUN: | FileCheck --check-prefixes CHECK,NOFUNCTION,CUSTOM,NOTYPED %s
+// RUN: %clang_cc1 -fxray-instrument \
+// RUN: -fxray-instrumentation-bundle=typed -x c++ \
+// RUN: -std=c++11 -triple x86_64-unknown-unknown -emit-llvm -o - %s \
+// RUN: | FileCheck --check-prefixes CHECK,NOFUNCTION,NOCUSTOM,TYPED %s
+// RUN: %clang_cc1 -fxray-instrument \
+// RUN: -fxray-instrumentation-bundle=custom,typed -x c++ \
+// RUN: -std=c++11 -triple x86_64-unknown-unknown -emit-llvm -o - %s \
+// RUN: | FileCheck --check-prefixes CHECK,NOFUNCTION,CUSTOM,TYPED %s
// RUN: %clang_cc1 -fxray-instrument \
// RUN: -fxray-instrumentation-bundle=function,custom -x c++ \
// RUN: -std=c++11 -triple x86_64-unknown-unknown -emit-llvm -o - %s \
-// RUN: | FileCheck --check-prefixes CHECK,FUNCTION,CUSTOM %s
+// RUN: | FileCheck --check-prefixes CHECK,FUNCTION,CUSTOM,NOTYPED %s
+// RUN: %clang_cc1 -fxray-instrument \
+// RUN: -fxray-instrumentation-bundle=function,typed -x c++ \
+// RUN: -std=c++11 -triple x86_64-unknown-unknown -emit-llvm -o - %s \
+// RUN: | FileCheck --check-prefixes CHECK,FUNCTION,NOCUSTOM,TYPED %s
+// RUN: %clang_cc1 -fxray-instrument \
+// RUN: -fxray-instrumentation-bundle=function,custom,typed -x c++ \
+// RUN: -std=c++11 -triple x86_64-unknown-unknown -emit-llvm -o - %s \
+// RUN: | FileCheck --check-prefixes CHECK,FUNCTION,CUSTOM,TYPED %s
// RUN: %clang_cc1 -fxray-instrument \
// RUN: -fxray-instrumentation-bundle=function \
-// RUN: -fxray-instrumentation-bundle=
[PATCH] D45716: [XRay] Add clang builtin for xray typed events.
kpw added a comment. My editor got a bit carried away with automatically clang-formatting lib/CodeGen/CodeGenFunction.cpp. I'll fix that so that I'm not messing up the revision history. Repository: rC Clang https://reviews.llvm.org/D45716 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45716: [XRay] Add clang builtin for xray typed events.
kpw updated this revision to Diff 142790.
kpw added a comment.
Undoing formatting change.
Repository:
rC Clang
https://reviews.llvm.org/D45716
Files:
include/clang/Basic/Builtins.def
include/clang/Basic/LangOptions.def
include/clang/Basic/XRayInstr.h
include/clang/Driver/Options.td
include/clang/Driver/XRayArgs.h
include/clang/Frontend/CodeGenOptions.def
lib/Basic/XRayInstr.cpp
lib/CodeGen/CGBuiltin.cpp
lib/CodeGen/CodeGenFunction.cpp
lib/CodeGen/CodeGenFunction.h
lib/Driver/XRayArgs.cpp
lib/Frontend/CompilerInvocation.cpp
test/CodeGen/xray-always-emit-typedevent.cpp
test/CodeGen/xray-instrumentation-bundles.cpp
test/CodeGen/xray-typedevent.cpp
Index: test/CodeGen/xray-typedevent.cpp
===
--- /dev/null
+++ test/CodeGen/xray-typedevent.cpp
@@ -0,0 +1,34 @@
+// RUN: %clang_cc1 -fxray-instrument -x c++ -std=c++11 -triple x86_64-unknown-unknown -emit-llvm -o - %s | FileCheck %s
+
+// CHECK-LABEL: @_Z16alwaysInstrumentv
+[[clang::xray_always_instrument]] void alwaysInstrument() {
+ // Event types would normally come from calling __xray_register_event_type
+ // from compiler-rt
+ auto EventType = 1;
+ static constexpr char kPhase[] = "instrument";
+ __xray_typedevent(EventType, kPhase, 10);
+ // CHECK: call void @llvm.xray.typedevent(i16 {{.*}}, i8*{{.*}}, i32 10)
+}
+
+// CHECK-LABEL: @_Z15neverInstrumentv
+[[clang::xray_never_instrument]] void neverInstrument() {
+ auto EventType = 2;
+ static constexpr char kPhase[] = "never";
+ __xray_typedevent(EventType, kPhase, 5);
+ // CHECK-NOT: call void @llvm.xray.typedevent(i16 {{.*}}, i8*{{.*}}, i32 5)
+}
+
+// CHECK-LABEL: @_Z21conditionalInstrumenti
+[[clang::xray_always_instrument]] void conditionalInstrument(int v) {
+ auto TrueEventType = 3;
+ auto UntrueEventType = 4;
+ static constexpr char kTrue[] = "true";
+ static constexpr char kUntrue[] = "untrue";
+ if (v % 2)
+__xray_typedevent(TrueEventType, kTrue, 4);
+ else
+__xray_typedevent(UntrueEventType, kUntrue, 6);
+
+ // CHECK: call void @llvm.xray.typedevent(i16 {{.*}}, i8*{{.*}}, i32 4)
+ // CHECK: call void @llvm.xray.typedevent(i16 {{.*}}, i8*{{.*}}, i32 6)
+}
Index: test/CodeGen/xray-instrumentation-bundles.cpp
===
--- test/CodeGen/xray-instrumentation-bundles.cpp
+++ test/CodeGen/xray-instrumentation-bundles.cpp
@@ -1,30 +1,49 @@
// RUN: %clang_cc1 -fxray-instrument -fxray-instrumentation-bundle=none -x c++ \
// RUN: -std=c++11 -triple x86_64-unknown-unknown -emit-llvm -o - %s \
-// RUN: | FileCheck --check-prefixes CHECK,NOFUNCTION,NOCUSTOM %s
-// RUN: %clang_cc1 -fxray-instrument \
-// RUN: -fxray-instrumentation-bundle=function -x c++ \
+// RUN: | FileCheck --check-prefixes CHECK,NOFUNCTION,NOCUSTOM,NOTYPED %s
+// RUN: %clang_cc1 -fxray-instrument -fxray-instrumentation-bundle=function -x c++ \
// RUN: -std=c++11 -triple x86_64-unknown-unknown -emit-llvm -o - %s \
-// RUN: | FileCheck --check-prefixes CHECK,FUNCTION,NOCUSTOM %s
+// RUN: | FileCheck --check-prefixes CHECK,FUNCTION,NOCUSTOM,NOTYPED %s
// RUN: %clang_cc1 -fxray-instrument \
// RUN: -fxray-instrumentation-bundle=custom -x c++ \
// RUN: -std=c++11 -triple x86_64-unknown-unknown -emit-llvm -o - %s \
-// RUN: | FileCheck --check-prefixes CHECK,NOFUNCTION,CUSTOM %s
+// RUN: | FileCheck --check-prefixes CHECK,NOFUNCTION,CUSTOM,NOTYPED %s
+// RUN: %clang_cc1 -fxray-instrument \
+// RUN: -fxray-instrumentation-bundle=typed -x c++ \
+// RUN: -std=c++11 -triple x86_64-unknown-unknown -emit-llvm -o - %s \
+// RUN: | FileCheck --check-prefixes CHECK,NOFUNCTION,NOCUSTOM,TYPED %s
+// RUN: %clang_cc1 -fxray-instrument \
+// RUN: -fxray-instrumentation-bundle=custom,typed -x c++ \
+// RUN: -std=c++11 -triple x86_64-unknown-unknown -emit-llvm -o - %s \
+// RUN: | FileCheck --check-prefixes CHECK,NOFUNCTION,CUSTOM,TYPED %s
// RUN: %clang_cc1 -fxray-instrument \
// RUN: -fxray-instrumentation-bundle=function,custom -x c++ \
// RUN: -std=c++11 -triple x86_64-unknown-unknown -emit-llvm -o - %s \
-// RUN: | FileCheck --check-prefixes CHECK,FUNCTION,CUSTOM %s
+// RUN: | FileCheck --check-prefixes CHECK,FUNCTION,CUSTOM,NOTYPED %s
+// RUN: %clang_cc1 -fxray-instrument \
+// RUN: -fxray-instrumentation-bundle=function,typed -x c++ \
+// RUN: -std=c++11 -triple x86_64-unknown-unknown -emit-llvm -o - %s \
+// RUN: | FileCheck --check-prefixes CHECK,FUNCTION,NOCUSTOM,TYPED %s
+// RUN: %clang_cc1 -fxray-instrument \
+// RUN: -fxray-instrumentation-bundle=function,custom,typed -x c++ \
+// RUN: -std=c++11 -triple x86_64-unknown-unknown -emit-llvm -o - %s \
+// RUN: | FileCheck --check-prefixes CHECK,FUNCTION,CUSTOM,TYPED %s
// RUN: %clang_cc1 -fxray-instrument \
// RUN: -fxray-instrumentation-bundle=function \
-// RUN: -fxray-instrumentation-bundle=custo
[PATCH] D45716: [XRay] Add clang builtin for xray typed events.
This revision was automatically updated to reflect the committed changes.
Closed by commit rC330220: [XRay] Add clang builtin for xray typed events.
(authored by kpw, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D45716?vs=142790&id=142836#toc
Repository:
rC Clang
https://reviews.llvm.org/D45716
Files:
include/clang/Basic/Builtins.def
include/clang/Basic/LangOptions.def
include/clang/Basic/XRayInstr.h
include/clang/Driver/Options.td
include/clang/Driver/XRayArgs.h
include/clang/Frontend/CodeGenOptions.def
lib/Basic/XRayInstr.cpp
lib/CodeGen/CGBuiltin.cpp
lib/CodeGen/CodeGenFunction.cpp
lib/CodeGen/CodeGenFunction.h
lib/Driver/XRayArgs.cpp
lib/Frontend/CompilerInvocation.cpp
test/CodeGen/xray-always-emit-typedevent.cpp
test/CodeGen/xray-instrumentation-bundles.cpp
test/CodeGen/xray-typedevent.cpp
Index: include/clang/Driver/Options.td
===
--- include/clang/Driver/Options.td
+++ include/clang/Driver/Options.td
@@ -1119,6 +1119,12 @@
def fnoxray_always_emit_customevents : Flag<["-"], "fno-xray-always-emit-customevents">, Group,
Flags<[CC1Option]>;
+def fxray_always_emit_typedevents : Flag<["-"], "fxray-always-emit-typedevents">, Group,
+ Flags<[CC1Option]>,
+ HelpText<"Determine whether to always emit __xray_typedevent(...) calls even if the function it appears in is not always instrumented.">;
+def fnoxray_always_emit_typedevents : Flag<["-"], "fno-xray-always-emit-typedevents">, Group,
+ Flags<[CC1Option]>;
+
def fxray_link_deps : Flag<["-"], "fxray-link-deps">, Group,
Flags<[CC1Option]>,
HelpText<"Tells clang to add the link dependencies for XRay.">;
Index: include/clang/Driver/XRayArgs.h
===
--- include/clang/Driver/XRayArgs.h
+++ include/clang/Driver/XRayArgs.h
@@ -29,6 +29,7 @@
bool XRayInstrument = false;
int InstructionThreshold = 200;
bool XRayAlwaysEmitCustomEvents = false;
+ bool XRayAlwaysEmitTypedEvents = false;
bool XRayRT = true;
public:
Index: include/clang/Basic/LangOptions.def
===
--- include/clang/Basic/LangOptions.def
+++ include/clang/Basic/LangOptions.def
@@ -281,6 +281,9 @@
LANGOPT(XRayAlwaysEmitCustomEvents, 1, 0,
"controls whether to always emit intrinsic calls to "
"__xray_customevent(...) builtin.")
+LANGOPT(XRayAlwaysEmitTypedEvents, 1, 0,
+"controls whether to always emit intrinsic calls to "
+"__xray_typedevent(...) builtin.")
BENIGN_LANGOPT(AllowEditorPlaceholders, 1, 0,
"allow editor placeholders in source")
@@ -298,4 +301,3 @@
#undef VALUE_LANGOPT
#undef COMPATIBLE_VALUE_LANGOPT
#undef BENIGN_VALUE_LANGOPT
-
Index: include/clang/Basic/XRayInstr.h
===
--- include/clang/Basic/XRayInstr.h
+++ include/clang/Basic/XRayInstr.h
@@ -31,13 +31,15 @@
enum XRayInstrOrdinal : XRayInstrMask {
XRIO_Function,
XRIO_Custom,
+ XRIO_Typed,
XRIO_Count
};
constexpr XRayInstrMask None = 0;
constexpr XRayInstrMask Function = 1U << XRIO_Function;
constexpr XRayInstrMask Custom = 1U << XRIO_Custom;
-constexpr XRayInstrMask All = Function | Custom;
+constexpr XRayInstrMask Typed = 1U << XRIO_Typed;
+constexpr XRayInstrMask All = Function | Custom | Typed;
} // namespace XRayInstrKind
Index: include/clang/Basic/Builtins.def
===
--- include/clang/Basic/Builtins.def
+++ include/clang/Basic/Builtins.def
@@ -1455,6 +1455,7 @@
// Builtins for XRay
BUILTIN(__xray_customevent, "vcC*z", "")
+BUILTIN(__xray_typedevent, "vzcC*z", "")
// Win64-compatible va_list functions
BUILTIN(__builtin_ms_va_start, "vc*&.", "nt")
Index: include/clang/Frontend/CodeGenOptions.def
===
--- include/clang/Frontend/CodeGenOptions.def
+++ include/clang/Frontend/CodeGenOptions.def
@@ -96,6 +96,9 @@
///< Set when -fxray-always-emit-customevents is enabled.
CODEGENOPT(XRayAlwaysEmitCustomEvents , 1, 0)
+///< Set when -fxray-always-emit-typedevents is enabled.
+CODEGENOPT(XRayAlwaysEmitTypedEvents , 1, 0)
+
///< Set the minimum number of instructions in a function to determine selective
///< XRay instrumentation.
VALUE_CODEGENOPT(XRayInstructionThreshold , 32, 200)
Index: test/CodeGen/xray-always-emit-typedevent.cpp
===
--- test/CodeGen/xray-always-emit-typedevent.cpp
+++ test/CodeGen/xray-always-emit-typedevent.cpp
@@ -0,0 +1,10 @@
+// RUN: %clang_cc1 -fxray-instrument -fxray-always-emit-typedevents -x c++ \
+// RUN: -std=c++11 -triple x86_64-unknown-unknown -emit-llvm -o - %s \
+// RUN: | FileCheck %s
+
+// CHECK-LABEL: @_Z15neverInstrumentv
+[[clang::xray_never_instrument]] void neverInstrument() {
+ stat
[PATCH] D45716: [XRay] Add clang builtin for xray typed events.
This revision was automatically updated to reflect the committed changes.
Closed by commit rL330220: [XRay] Add clang builtin for xray typed events.
(authored by kpw, committed by ).
Repository:
rC Clang
https://reviews.llvm.org/D45716
Files:
cfe/trunk/include/clang/Basic/Builtins.def
cfe/trunk/include/clang/Basic/LangOptions.def
cfe/trunk/include/clang/Basic/XRayInstr.h
cfe/trunk/include/clang/Driver/Options.td
cfe/trunk/include/clang/Driver/XRayArgs.h
cfe/trunk/include/clang/Frontend/CodeGenOptions.def
cfe/trunk/lib/Basic/XRayInstr.cpp
cfe/trunk/lib/CodeGen/CGBuiltin.cpp
cfe/trunk/lib/CodeGen/CodeGenFunction.cpp
cfe/trunk/lib/CodeGen/CodeGenFunction.h
cfe/trunk/lib/Driver/XRayArgs.cpp
cfe/trunk/lib/Frontend/CompilerInvocation.cpp
cfe/trunk/test/CodeGen/xray-always-emit-typedevent.cpp
cfe/trunk/test/CodeGen/xray-instrumentation-bundles.cpp
cfe/trunk/test/CodeGen/xray-typedevent.cpp
Index: cfe/trunk/test/CodeGen/xray-instrumentation-bundles.cpp
===
--- cfe/trunk/test/CodeGen/xray-instrumentation-bundles.cpp
+++ cfe/trunk/test/CodeGen/xray-instrumentation-bundles.cpp
@@ -1,30 +1,49 @@
// RUN: %clang_cc1 -fxray-instrument -fxray-instrumentation-bundle=none -x c++ \
// RUN: -std=c++11 -triple x86_64-unknown-unknown -emit-llvm -o - %s \
-// RUN: | FileCheck --check-prefixes CHECK,NOFUNCTION,NOCUSTOM %s
-// RUN: %clang_cc1 -fxray-instrument \
-// RUN: -fxray-instrumentation-bundle=function -x c++ \
+// RUN: | FileCheck --check-prefixes CHECK,NOFUNCTION,NOCUSTOM,NOTYPED %s
+// RUN: %clang_cc1 -fxray-instrument -fxray-instrumentation-bundle=function -x c++ \
// RUN: -std=c++11 -triple x86_64-unknown-unknown -emit-llvm -o - %s \
-// RUN: | FileCheck --check-prefixes CHECK,FUNCTION,NOCUSTOM %s
+// RUN: | FileCheck --check-prefixes CHECK,FUNCTION,NOCUSTOM,NOTYPED %s
// RUN: %clang_cc1 -fxray-instrument \
// RUN: -fxray-instrumentation-bundle=custom -x c++ \
// RUN: -std=c++11 -triple x86_64-unknown-unknown -emit-llvm -o - %s \
-// RUN: | FileCheck --check-prefixes CHECK,NOFUNCTION,CUSTOM %s
+// RUN: | FileCheck --check-prefixes CHECK,NOFUNCTION,CUSTOM,NOTYPED %s
+// RUN: %clang_cc1 -fxray-instrument \
+// RUN: -fxray-instrumentation-bundle=typed -x c++ \
+// RUN: -std=c++11 -triple x86_64-unknown-unknown -emit-llvm -o - %s \
+// RUN: | FileCheck --check-prefixes CHECK,NOFUNCTION,NOCUSTOM,TYPED %s
+// RUN: %clang_cc1 -fxray-instrument \
+// RUN: -fxray-instrumentation-bundle=custom,typed -x c++ \
+// RUN: -std=c++11 -triple x86_64-unknown-unknown -emit-llvm -o - %s \
+// RUN: | FileCheck --check-prefixes CHECK,NOFUNCTION,CUSTOM,TYPED %s
// RUN: %clang_cc1 -fxray-instrument \
// RUN: -fxray-instrumentation-bundle=function,custom -x c++ \
// RUN: -std=c++11 -triple x86_64-unknown-unknown -emit-llvm -o - %s \
-// RUN: | FileCheck --check-prefixes CHECK,FUNCTION,CUSTOM %s
+// RUN: | FileCheck --check-prefixes CHECK,FUNCTION,CUSTOM,NOTYPED %s
+// RUN: %clang_cc1 -fxray-instrument \
+// RUN: -fxray-instrumentation-bundle=function,typed -x c++ \
+// RUN: -std=c++11 -triple x86_64-unknown-unknown -emit-llvm -o - %s \
+// RUN: | FileCheck --check-prefixes CHECK,FUNCTION,NOCUSTOM,TYPED %s
+// RUN: %clang_cc1 -fxray-instrument \
+// RUN: -fxray-instrumentation-bundle=function,custom,typed -x c++ \
+// RUN: -std=c++11 -triple x86_64-unknown-unknown -emit-llvm -o - %s \
+// RUN: | FileCheck --check-prefixes CHECK,FUNCTION,CUSTOM,TYPED %s
// RUN: %clang_cc1 -fxray-instrument \
// RUN: -fxray-instrumentation-bundle=function \
-// RUN: -fxray-instrumentation-bundle=custom -x c++ \
+// RUN: -fxray-instrumentation-bundle=custom \
+// RUN: -fxray-instrumentation-bundle=typed -x c++ \
// RUN: -std=c++11 -triple x86_64-unknown-unknown -emit-llvm -o - %s \
-// RUN: | FileCheck --check-prefixes CHECK,FUNCTION,CUSTOM %s
+// RUN: | FileCheck --check-prefixes CHECK,FUNCTION,CUSTOM,TYPED %s
// CHECK: define void @_Z16alwaysInstrumentv() #[[ALWAYSATTR:[0-9]+]] {
[[clang::xray_always_instrument]] void alwaysInstrument() {
static constexpr char kPhase[] = "always";
__xray_customevent(kPhase, 6);
+ __xray_typedevent(1, kPhase, 6);
// CUSTOM: call void @llvm.xray.customevent(i8*{{.*}}, i32 6)
// NOCUSTOM-NOT: call void @llvm.xray.customevent(i8*{{.*}}, i32 6)
+ // TYPED: call void @llvm.xray.typedevent(i16 {{.*}}, i8*{{.*}}, i32 6)
+ // NOTYPED-NOT: call void @llvm.xray.typedevent(i16 {{.*}}, i8*{{.*}}, i32 6)
}
// FUNCTION: attributes #[[ALWAYSATTR]] = {{.*}} "function-instrument"="xray-always" {{.*}}
Index: cfe/trunk/test/CodeGen/xray-always-emit-typedevent.cpp
===
--- cfe/trunk/test/CodeGen/xray-always-emit-typedevent.cpp
+++ cfe/trunk/test/CodeGen/xray-always-emit-typedevent.cpp
@@ -0,0 +1,10 @@
+// RUN: %clang_cc1
[PATCH] D86089: [flang][driver]Add experimental flang driver and frontend with help screen
tskeith requested changes to this revision.
tskeith added inline comments.
This revision now requires changes to proceed.
Comment at: flang/include/flang/Frontend/CompilerInstance.h:11
+
+#include "flang/Frontend/CompilerInvocation.h"
+
Why is this called "Frontend" rather than "Driver"?
Comment at: flang/include/flang/Frontend/CompilerInstance.h:16
+
+namespace flang {
+
Nothing else is in namespace `flang`. `Fortran::frontend` would be more
consistent.
Comment at: flang/include/flang/Frontend/CompilerInstance.h:21
+ /// The options used in this compiler instance.
+ std::shared_ptr Invocation;
+
Data members, local variables, etc. should start with lower case.
Non-public data members should end with underscore.
Please follow the style in flang/documentation/C++style.md.
Comment at: flang/include/flang/Frontend/FrontendOptions.h:31
+ Language Lang;
+ unsigned Fmt : 3;
+ unsigned Preprocessed : 1;
Why isn't the type `Format`?
Comment at: flang/include/flang/Frontend/FrontendOptions.h:32
+ unsigned Fmt : 3;
+ unsigned Preprocessed : 1;
+
Why isn't the type `bool`?
Comment at: flang/include/flang/Frontend/FrontendOptions.h:65
+ /// Show the -version text.
+ unsigned ShowVersion : 1;
+
Why aren't the types `bool`?
Comment at: flang/include/flang/FrontendTool/Utils.h:18
+
+#include
+
Is this needed?
Comment at: flang/lib/Frontend/CompilerInstance.cpp:39
+ } else
+Diags->setClient(new clang::TextDiagnosticPrinter(llvm::errs(), Opts));
+
The "then" and "else" statements should have braces around them.
Comment at: flang/lib/Frontend/CompilerInvocation.cpp:46
+ InputKind DashX(Language::Unknown);
+ return DashX;
+}
Why not `return InputKind{Language::Unknown};`?
Comment at: flang/lib/Frontend/CompilerInvocation.cpp:91
+ InputKind DashX = ParseFrontendArgs(Res.getFrontendOpts(), Args, Diags);
+ (void)DashX;
+
What is the purpose of `DashX` here?
Comment at: flang/lib/FrontendTool/ExecuteCompilerInvocation.cpp:20
+
+using namespace flang;
+namespace flang {
What is this for?
Comment at: flang/tools/flang-driver/driver.cpp:30
+extern int fc1_main(
+llvm::ArrayRef Argv, const char *Argv0, void *MainAddr);
+
Why isn't this declared in a header?
Comment at: flang/tools/flang-driver/fc1_main.cpp:32
+int fc1_main(
+llvm::ArrayRef Argv, const char *Argv0, void *MainAddr) {
+
MainAddr isn't used.
Comment at: flang/tools/flang-driver/fc1_main.cpp:54
+ // Execute the frontend actions.
+ { Success = ExecuteCompilerInvocation(Flang.get()); }
+
Why is this statement in a block? Is the result of CreateFromArgs supposed to
affect the return value of this function?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D86089/new/
https://reviews.llvm.org/D86089
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D93401: [flang][driver] Add support for `-D`, `-U`
tskeith added inline comments.
Comment at: flang/lib/Frontend/CompilerInvocation.cpp:228
+ // Note: GCC drops anything following an end-of-line character.
+ llvm::StringRef::size_type End = MacroBody.find_first_of("\n\r");
+ MacroBody = MacroBody.substr(0, End);
This is a change in behavior from f18, right? If so, the commit message should
mention it and why it's changing.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D93401/new/
https://reviews.llvm.org/D93401
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D95460: [flang][driver] Add forced form flags and -ffixed-line-length
tskeith added inline comments. Comment at: clang/include/clang/Driver/Options.td:4133 def fconvert_EQ : Joined<["-"], "fconvert=">, Group; -def ffixed_line_length_VALUE : Joined<["-"], "ffixed-line-length-">, Group; +def ffixed_line_length_VALUE : Joined<["-"], "ffixed-line-length-">, Flags<[FlangOption, FC1Option]>, + Group, HelpText<"Use as character line width in fixed mode">, Why isn't this `-ffixed-line-length=` (i.e. with an equals sign, not a hyphen)? Isn't that how clang does options with values? It's fine to support the gfortran form as an alternative but I think the primary goal should be good consistent style for options. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D95460/new/ https://reviews.llvm.org/D95460 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D95448: [flang][driver] Add support for `-J/-module-dir`
tskeith added inline comments.
Comment at: clang/include/clang/Driver/Options.td:1018
+ an USE statement. The default is the current
directory.}]>,Group;
+def module_dir : Separate<["-"], "module-dir">,
Flags<[FlangOption,FC1Option]>, MetaVarName<"">, Alias;
def dsym_dir : JoinedOrSeparate<["-"], "dsym-dir">,
It would be better to make `-module-dir` the main option and `-J` the alias.
`-J` is only there for gfortran compatibility, not because it is a good name
for the option.
Comment at: flang/include/flang/Frontend/CompilerInstance.h:105
/// {
+ Fortran::semantics::SemanticsContext &semaChecking() const { return
*semantics_; }
`semanticsContext` would be a better name for this function.
Comment at: flang/include/flang/Frontend/CompilerInvocation.h:67
+ // of options.
+ std::string moduleDirJ_ = ".";
+
`moduleDir_` would be a better name.
Comment at: flang/lib/Frontend/CompilerInstance.cpp:29
+ semantics_(new Fortran::semantics::SemanticsContext(*(new
Fortran::common::IntrinsicTypeDefaultKinds()),*(new
common::LanguageFeatureControl()),
+ *allCookedSources_)) {
Why is `semantics_` a `shared_ptr` rather than a simple data member of type
`SemanticsContext`? It's owned by only `CompilerInstance`, not shared.
The same question probably applies to the other fields too.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D95448/new/
https://reviews.llvm.org/D95448
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D95448: [flang][driver] Add support for `-J/-module-dir`
tskeith added inline comments.
Comment at: flang/include/flang/Frontend/CompilerInstance.h:105
/// {
+ Fortran::semantics::SemanticsContext &semaChecking() const { return
*semantics_; }
awarzynski wrote:
> tskeith wrote:
> > `semanticsContext` would be a better name for this function.
> We should follow Flang's [[
> https://github.com/llvm/llvm-project/blob/main/flang/docs/C%2B%2Bstyle.md#naming
> | coding style ]] here:
> ```
> Accessor member functions are named with the non-public data member's name,
> less the trailing underscore.
> ```
> i.e. `semantics()` rather than `semanticsContext()`. If we were to diverge
> from that, then I suggest that we follow the style prevalent in LLVM/Clang,
> see e.g. [[
> https://github.com/llvm/llvm-project/blob/21bfd068b32ece1c6fbc912208e7cd1782a8c3fc/clang/include/clang/Frontend/CompilerInstance.h#L503-L506
> | getSema ]].
>
> @tskeith, I'm guessing that you wanted the member variable to be updated too:
> * semantics_ -> semanticsContext_
> Makes sense to me.
>
> @tskeith, I'm guessing that you wanted the member variable to be updated too:
> * semantics_ -> semanticsContext_
Right.
Comment at: flang/lib/Frontend/CompilerInstance.cpp:29
+ semantics_(new Fortran::semantics::SemanticsContext(*(new
Fortran::common::IntrinsicTypeDefaultKinds()),*(new
common::LanguageFeatureControl()),
+ *allCookedSources_)) {
awarzynski wrote:
> tskeith wrote:
> > Why is `semantics_` a `shared_ptr` rather than a simple data member of
> > type `SemanticsContext`? It's owned by only `CompilerInstance`, not shared.
> > The same question probably applies to the other fields too.
> @tskeith You raise two important points here:
>
> **Why shared_ptr if the resource is not shared?**
> From what I can see, at this point the `SemanticsContext` is not shared and
> we can safely use `unique_ptr` instead.
>
> **Why are semantics_ and other members of CompilerInstance pointers?**
> `CompilerInstance` doesn't really own much - it just encapsulates all
> classes/structs that are required for creating a _compiler instance_. It's
> kept lightweight and written in a way that makes it easy to _inject_ custom
> instances of these classes. This approach is expected to be helpful when
> creating new frontend actions (I expect that there will be a lot) and when
> compiling projects with many source files.
>
> @tskeith, I intend to document the design of the new driver soon and suggest
> that that's when we re-open the discussion on the design of
> `CompilerInstance`.
>
> IMO this change is consistent with the current design and I think that we
> should accept it as is.
>
> **Small suggestion**
> @arnamoy10 , I think that you can safely add `IntrinsicTypeDefaultKinds` and
> `LanguageFeatureControl` members too. We will need them shortly and this way
> this constructor becomes much cleaner. I'm fine either way!
>
>
> **Why are semantics_ and other members of CompilerInstance pointers?**
> `CompilerInstance` doesn't really own much - it just encapsulates all
> classes/structs that are required for creating a _compiler instance_.
As it stands it does own the instance of `SemanticsContext` etc. No one else
does.
> @tskeith, I intend to document the design of the new driver soon and suggest
> that that's when we re-open the discussion on the design of
> `CompilerInstance`.
OK
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D95448/new/
https://reviews.llvm.org/D95448
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D96483: [flang][driver] Add options for unparsing
tskeith added inline comments.
Comment at: flang/tools/f18/f18.cpp:541
driver.debugNoSemantics = true;
-} else if (arg == "-funparse") {
+} else if (arg == "-funparse" || arg == "-fdebug_unparse") {
driver.dumpUnparse = true;
This should be "-fdebug-unparse" (with a hyphen). That may be why all those
tests are failing.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D96483/new/
https://reviews.llvm.org/D96483
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D96344: [flang][driver] Add options for -fdefault* and -flarge-sizes
tskeith added inline comments. Comment at: clang/include/clang/Driver/Options.td:4231 +def flarge_sizes : Flag<["-"],"flarge-sizes">, Group, + HelpText<"Set the default KIND for INTEGER to 8.">; } That's not what -flarge-sizes does. Here is the description from flang/docs/Extensions.md: > The default `INTEGER` type is required by the standard to occupy > the same amount of storage as the default `REAL` type. Default > `REAL` is of course 32-bit IEEE-754 floating-point today. This legacy > rule imposes an artificially small constraint in some cases > where Fortran mandates that something have the default `INTEGER` > type: specifically, the results of references to the intrinsic functions > `SIZE`, `STORAGE_SIZE`,`LBOUND`, `UBOUND`, `SHAPE`, and the location > reductions > `FINDLOC`, `MAXLOC`, and `MINLOC` in the absence of an explicit > `KIND=` actual argument. We return `INTEGER(KIND=8)` by default in > these cases when the `-flarge-sizes` option is enabled. > `SIZEOF` and `C_SIZEOF` always return `INTEGER(KIND=8)`. Comment at: flang/lib/Frontend/CompilerInvocation.cpp:269 +} +res.defaultKinds().set_defaultRealKind(4); + } If `-fdefault-double-8` requires `-fdefault-real-8` then why is the default real kind set to 4? I don't understand the purpose of this option. The kind of DOUBLE PRECISION is already 8 so there is no need to change it. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96344/new/ https://reviews.llvm.org/D96344 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D96875: [flang][driver] Add -fdebug-module-writer option
tskeith added inline comments. Comment at: flang/lib/Frontend/CompilerInvocation.cpp:234 + // -J/module-dir option + auto &moduleDir = res.moduleDir(); auto moduleDirList = In my opinion, using mutable references like this make the code hard to understand. Setters on CompilerInvocation would be clearer. Comment at: flang/test/Semantics/mod-file-rewriter.f90:9 +! RUN: %flang-new -fsyntax-only -fdebug-module-writer %p/Inputs/mod-file-unchanged.f90 2>&1 | FileCheck %s --check-prefix CHECK_UNCHANGED +! RUN: %flang-new -fsyntax-only -fdebug-module-writer %p/Inputs/mod-file-changed.f90 2>&1 | FileCheck %s --check-prefix CHECK_CHANGED Why do you need to duplicate these lines? Doesn't it work to use `%flang`? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96875/new/ https://reviews.llvm.org/D96875 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D96344: [flang][driver] Add options for -fdefault* and -flarge-sizes
tskeith added inline comments.
Comment at: flang/lib/Frontend/CompilerInvocation.cpp:254
+ if (args.hasArg(clang::driver::options::OPT_fdefault_real_8))
+res.defaultKinds().set_defaultRealKind(8);
+ if (args.hasArg(clang::driver::options::OPT_fdefault_integer_8)) {
awarzynski wrote:
> arnamoy10 wrote:
> > awarzynski wrote:
> > > From `gfortran` [[
> > > https://gcc.gnu.org/onlinedocs/gfortran/Fortran-Dialect-Options.html |
> > > documentation ]]:
> > >
> > > ```
> > > This option promotes the default width of DOUBLE PRECISION and double
> > > real constants like 1.d0 to 16 bytes if possible.
> > > ```
> > >
> > > So I believe that you are missing:
> > > ```
> > > res.defaultKinds().set_doublePrecisionKind(16);
> > > ```
> > I thought it is not necessary because `doublePrecisionKind` is
> > automatically initialized to double the size of `defaultRealKind_` in [[
> > https://github.com/llvm/llvm-project/blob/adfd3c7083f9808d145239153c10f72eece485d8/flang/include/flang/Common/default-kinds.h#L51-L58
> > | here ]]--> `int doublePrecisionKind_{2 * defaultRealKind_};`.
> >
> >
> Yes, but the default value for `defaultRealKind_` is 4 and here you are
> setting it to 8. So, correct me if I'm wrong, but when the driver is here,
> the following has happened:
> ```
> int defaultIntegerKind_{4};
> int defaultRealKind_{defaultIntegerKind_};
> int doublePrecisionKind_{2 * defaultRealKind_};
> ```
> So `dublePrecisionKind_` is 8 rather than 16.
You can test these are working correctly by compiling a module like this:
```
module m
implicit none
real :: x
double precision :: y
integer, parameter :: real_kind = kind(x)
integer, parameter :: double_kind = kind(y)
end
```
Then check for `double_kind=8_4` (or whatever) in the generated `.mod` file.
For `-flarge-sizes`:
```
real :: z(10)
integer, parameter :: size_kind = kind(ubound(z, 1))
```
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D96344/new/
https://reviews.llvm.org/D96344
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D96344: [flang][driver] Add options for -fdefault* and -flarge-sizes
tskeith added inline comments. Comment at: flang/test/Flang-Driver/fdefault.f90:4 + +! REQUIRES: new-flang-driver + Can't this work with the f18 driver too? That's the best way to ensure they are consistent. Comment at: flang/test/Flang-Driver/fdefault.f90:10 +! RUN: not %flang-new -fsyntax-only -fdefault-double-8 %s 2>&1 | FileCheck %s --check-prefix=DOUBLE +! RUN: mkdir -p %t/dir-flang-new && %flang-new -fsyntax-only -module-dir %t/dir-flang-new -fdefault-real-8 %s 2>&1 +! RUN: cat %t/dir-flang-new/m.mod | grep 'real_kind=8_4' I don't think you need to create a subdirectory. `%t` is a temp directory specific to this test. So I'm suggesting: ``` ! RUN: %flang -fsyntax-only -fdefault-real-8 %s ``` CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96344/new/ https://reviews.llvm.org/D96344 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D96344: [flang][driver] Add options for -fdefault* and -flarge-sizes
tskeith added inline comments. Comment at: flang/test/Flang-Driver/fdefault.f90:10 +! RUN: not %flang-new -fsyntax-only -fdefault-double-8 %s 2>&1 | FileCheck %s --check-prefix=DOUBLE +! RUN: mkdir -p %t/dir-flang-new && %flang-new -fsyntax-only -module-dir %t/dir-flang-new -fdefault-real-8 %s 2>&1 +! RUN: cat %t/dir-flang-new/m.mod | grep 'real_kind=8_4' awarzynski wrote: > tskeith wrote: > > I don't think you need to create a subdirectory. `%t` is a temp directory > > specific to this test. > > > > So I'm suggesting: > > ``` > > ! RUN: %flang -fsyntax-only -fdefault-real-8 %s > > ``` > From [[ https://llvm.org/docs/CommandGuide/lit.html#substitutions | LIT docs > ]]: > > ``` > %ttemporary file name unique to the test > %Tparent directory of %t (not unique, deprecated, do not use) > ``` > > So, IIUC, we do need to create a directory. We could skip `` > in the directory name, but it does no harm and can be really helpful when > parsing CI logs. > > I might be missing something though. @tskeith ? I thought lit created an empty directory for `%t` but apparently not. You just get whatever was left over from last run. So the safest thing to do is this for each compilation command: `! RUN: rm -rf %t && mkdir %t && %flang -module-dir %t ...` It's true that adding an extra subdirectory does no harm besides clutter, but what's the benefit? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96344/new/ https://reviews.llvm.org/D96344 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D96344: [flang][driver] Add options for -fdefault* and -flarge-sizes
tskeith added inline comments.
Comment at: flang/test/Flang-Driver/fdefault.f90:4
+
+! REQUIRES: new-flang-driver
+
awarzynski wrote:
> tskeith wrote:
> > Can't this work with the f18 driver too? That's the best way to ensure they
> > are consistent.
> I think that this is a good idea, but there are two things that need to be
> addressed first:
>
> **OPTION NAMES**
>
> `f18` uses `-module` rather than `-module-dir`. I suggest adding an alias,
> `-module-dir`, to `f18` (e.g. similarly to how `-fparse-only` and
> `-fsyntax-only` are handled)
>
> **IMPLEMENTATION**
> If I understand correctly, the current implementation of `-fdefault-real-8`
> in `f18` is incomplete, see [[
> https://github.com/llvm/llvm-project/blob/b6db47d7e044730dc3c9b35dae6697eee0885dbf/flang/tools/f18/f18.cpp#L568-L569
> | here ]]:
> ```
> } else if (arg == "-r8" || arg == "-fdefault-real-8") {
> defaultKinds.set_defaultRealKind(8);
> ```
> I think that there should be `defaultKinds.set_defaultRealKind(16);` as well.
>
> @tskeith could you confirm?
> I think that this is a good idea, but there are two things that need to be
> addressed first:
>
> **OPTION NAMES**
>
> `f18` uses `-module` rather than `-module-dir`. I suggest adding an alias,
> `-module-dir`, to `f18` (e.g. similarly to how `-fparse-only` and
> `-fsyntax-only` are handled)
Yes, in general when an option is given a different name in the new driver that
option should be added to f18. Not only for testing consistent behavior but
also because f18 gets more usage so any problems are more likely to turn up.
> **IMPLEMENTATION**
> If I understand correctly, the current implementation of `-fdefault-real-8`
> in `f18` is incomplete, see [[
> https://github.com/llvm/llvm-project/blob/b6db47d7e044730dc3c9b35dae6697eee0885dbf/flang/tools/f18/f18.cpp#L568-L569
> | here ]]:
> ```
> } else if (arg == "-r8" || arg == "-fdefault-real-8") {
> defaultKinds.set_defaultRealKind(8);
> ```
> I think that there should be `defaultKinds.set_defaultRealKind(16);` as well.
Do you mean `set_doublePrecisionKind(16)`? Yes, if that's how
`-fdefault-real-8` is supposed to behave, it should work that way in f18 too.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D96344/new/
https://reviews.llvm.org/D96344
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D97119: [flang][driver] Add options for -std=2018
tskeith added a comment.
Please make sure the test works with f18 also.
Comment at: flang/lib/Frontend/CompilerInvocation.cpp:294
+// We only allow 2018 as the given standard
+if (standard.equals("2018")) {
+ res.SetStandard();
This should be "f2018", not "2018".
Comment at: flang/test/Flang-Driver/std2018.f90:18
+!-
+! GIVEN: A DO loop should terminate with an END DO or CONTINUE
+
Can you verify this message is not produced when there is no -std option?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D97119/new/
https://reviews.llvm.org/D97119
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D96875: [flang][driver] Add -fdebug-module-writer option
tskeith accepted this revision. tskeith added a comment. LGTM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96875/new/ https://reviews.llvm.org/D96875 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D98191: [flang][driver] Add support for `-fdebug-dump-symbols-sources`
tskeith added a comment. `-fget-symbols-sources` is not a debug option, it's intended for integrating with IDEs like vscode. So I think the original name is better. Unlike the "dump" options it actually is an action and not something that is intended to produce debug output on the way to doing something else. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D98191/new/ https://reviews.llvm.org/D98191 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D97080: [flang][driver] Add -fintrinsic-modules-path option
tskeith added inline comments.
Comment at: flang/lib/Frontend/CompilerInvocation.cpp:293
+ driverPath = driverPath.substr(0, driverPath.size() - 9);
+ return driverPath.append("/../tools/flang/include/flang/");
+}
Does this work for an install? I think there the path would be `include/flang`
rather than `tools/flang/include/flang`.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D97080/new/
https://reviews.llvm.org/D97080
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D97080: [flang][driver] Add -fintrinsic-modules-path option
tskeith added inline comments.
Comment at: flang/lib/Frontend/CompilerInvocation.cpp:293
+ driverPath = driverPath.substr(0, driverPath.size() - 9);
+ return driverPath.append("/../tools/flang/include/flang/");
+}
arnamoy10 wrote:
> tskeith wrote:
> > Does this work for an install? I think there the path would be
> > `include/flang` rather than `tools/flang/include/flang`.
> You are probably right, I am giving the path w.r.t a build. Can we make the
> assumption that there should be always an install? Or do we determine if we
> flang-new is coming from build or install (by checking if a module file is
> present through ls) and then set the path accordingly?
I think it should work in a build or an install. The `flang/tools/f18/flang`
script checks for `include/flang` first and if that doesn't exist it uses
`tools/flang/include/flang` so you could do the same. It would be good if we
could fix the build or install so that the location is consistent.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D97080/new/
https://reviews.llvm.org/D97080
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D97080: [flang][driver] Add -fintrinsic-modules-path option
tskeith added inline comments.
Comment at: flang/lib/Frontend/CompilerInvocation.cpp:293
+ driverPath = driverPath.substr(0, driverPath.size() - 9);
+ return driverPath.append("/../tools/flang/include/flang/");
+}
tskeith wrote:
> arnamoy10 wrote:
> > tskeith wrote:
> > > Does this work for an install? I think there the path would be
> > > `include/flang` rather than `tools/flang/include/flang`.
> > You are probably right, I am giving the path w.r.t a build. Can we make
> > the assumption that there should be always an install? Or do we determine
> > if we flang-new is coming from build or install (by checking if a module
> > file is present through ls) and then set the path accordingly?
> I think it should work in a build or an install. The `flang/tools/f18/flang`
> script checks for `include/flang` first and if that doesn't exist it uses
> `tools/flang/include/flang` so you could do the same. It would be good if we
> could fix the build or install so that the location is consistent.
I've created D98522 to make the relative path be `include/flang` in both build
and install. If no one objects, that should make it so you don't need
conditional code here, just use `"/../include/flang"`.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D97080/new/
https://reviews.llvm.org/D97080
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D97119: [flang][driver] Add options for -std=f2018
tskeith added inline comments. Comment at: flang/test/Driver/std2018.f90:9 +! RUN: %flang_fc1 -pedantic %s 2>&1 | FileCheck %s --check-prefix=GIVEN +! RUN: not %flang_fc1 -std=90 %s 2>&1 | FileCheck %s --check-prefix=WRONG + You need to make sure these work with f18 or indicate the test requires the new driver. f18 doesn't currently support `-std=f2018` and doesn't complain about unrecognized options. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D97119/new/ https://reviews.llvm.org/D97119 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D98191: [flang][driver] Add support for `-fdebug-dump-symbols-sources`
tskeith added a comment. > Would this option be used to extract debug/code-navigation info? Yes, it's something related to mapping between symbols and source locations. > Is there an equivalent in `clang` or `gfortran`? Not that I know of. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D98191/new/ https://reviews.llvm.org/D98191 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D99292: [flang][driver] Add support for `-cpp/-nocpp`
tskeith added inline comments. Comment at: clang/include/clang/Driver/Options.td:4302 +def cpp : Flag<["-"], "cpp">, Group, + HelpText<"Always add standard macro predefinitions">; +def nocpp : Flag<["-"], "nocpp">, Group, This option affects command line macro definitions too. So maybe something like: `Enable predefined and command line preprocessor macros` Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99292/new/ https://reviews.llvm.org/D99292 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D93453: [flang][driver] Add support for `-I`
tskeith added inline comments. Comment at: flang/test/Flang-Driver/include-header.f90:59 +#endif +end `-I` also is supposed to affect INCLUDE lines so it would be good to have a test for that too. They are handled during preprocessing and so can be tested the same way. It also affects searching for .mod files but I think to test that requires semantics (i.e. to report an error if the module can't be found). It would be good to test that somewhere too. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93453/new/ https://reviews.llvm.org/D93453 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
