[PATCH] D47196: [Time-report ](2): Recursive timers in Clang
efriedma added a comment. I mean, which of the callers of startFrontendTimer() is calling it with a pointer to std::declval()? https://reviews.llvm.org/D47196 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45619: [Time-report] (1) Use special new Clang flag 'FrontendTimesIsEnabled' instead of 'llvm::TimePassesIsEnabled' inside -ftime-report feature
efriedma added a comment. Oh, oops, I should have spotted that. No, the point of this was to separate the timing infrastructure, not to change the behavior of -ftime-report. Should be a one-line change to add "llvm::TimePassesIsEnabled = TimePasses" back to BackendConsumer::BackendConsumer. (Maybe we can add flags to control them separately as a followup.) Repository: rL LLVM https://reviews.llvm.org/D45619 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50179: [AArch64][ARM] Context sensitive meaning of option "crypto"
efriedma added inline comments.
Comment at: lib/Driver/ToolChains/Arch/AArch64.cpp:266
+ const bool HasV83a = (std::find(ItBegin, ItEnd, "+v8.3a") != ItEnd);
+ const bool HasV84a = (std::find(ItBegin, ItEnd, "+v8.4a") != ItEnd);
+ const bool HasV85a = (std::find(ItBegin, ItEnd, "+v8.5a") != ItEnd);
HasV84a is always false; you checked it a few lines earlier. And I think that
implies HasV85a is also false? Not sure.
Comment at: lib/Driver/ToolChains/Arch/ARM.cpp:430
+ if (ArchName.find_lower("+noaes") == StringRef::npos)
+Features.push_back("+aes");
+} else if (ArchName.find_lower("-crypto") != StringRef::npos) {
SjoerdMeijer wrote:
> efriedma wrote:
> > The ARM backend doesn't support features named "sha2" and "aes" at the
> > moment.
> These ARM target features were introduced in rL335953.
Hmm, I missed that somehow; okay.
https://reviews.llvm.org/D50179
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50507: [CodeGen][ARM] Coerce FP16 vectors to integer vectors when needed
efriedma added a comment. Do we need to check for homogeneous aggregates of half vectors somewhere? https://reviews.llvm.org/D50507 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45619: [Time-report] (1) Use special new Clang flag 'FrontendTimesIsEnabled' instead of 'llvm::TimePassesIsEnabled' inside -ftime-report feature
efriedma added a comment. Somewhere around CompilerInvocation::CreateFromArgs is probably appropriate. Repository: rL LLVM https://reviews.llvm.org/D45619 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50507: [CodeGen][ARM] Coerce FP16 vectors to integer vectors when needed
efriedma added inline comments. Comment at: lib/CodeGen/TargetInfo.cpp:5788 + llvm::Type *Ty = llvm::ArrayType::get(NewVecTy, Members); + return ABIArgInfo::getDirect(Ty, 0, nullptr, false); +} Do we need equivalent code in classifyReturnType? https://reviews.llvm.org/D50507 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50683: [Android] Set NewAlign for 64-bit Android to 8 bytes
efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. LGTM Comment at: lib/Basic/TargetInfo.cpp:72 +// For 64-bit Android, alignment is 8 bytes for allocations <= 8 bytes. +NewAlign = (Triple.isArch64Bit() || Triple.isArch32Bit()) ? 64 : 0; + } else Might as well just set `NewAlign = 64;` here. But not a big deal either way. Repository: rC Clang https://reviews.llvm.org/D50683 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50683: [Android] Set NewAlign for 64-bit Android to 8 bytes
efriedma added a comment. The C++ standard just says "An integer literal of type std::size_t whose value is the alignment guaranteed by a call to operator new(std::size_t) or operator new[](std::size_t)." I would take that in the obvious sense, that any pointer returned by operator new must have alignment `__STDCPP_DEFAULT_NEW_ALIGNMENT__`. Granted, I can see how that might not be the intent, given that `alignof(T) >= sizeof(T)` for all C++ types. Repository: rC Clang https://reviews.llvm.org/D50683 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47196: [Time-report ](2): Recursive timers in Clang
efriedma added a comment. > :start: means the timer was started In some cases, the ChildTime is already non-zero at the "start" point; what does that mean? > ActOnFunctionDeclarator:start:._exception: That's weird... DeclarationName::print should generally return something ASCII; do you know what kind of declaration it is? https://reviews.llvm.org/D47196 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51011: [Preprocessor] raise gcc compatibility macros.
efriedma added a comment. I'd suggest something like this, if you really need to detect the compiler: #if defined(__clang__) // clang #elif defined(__INTEL_COMPILER) // icc #elif defined(__GNUC__) // gcc #else #error "Unknown compiler" #endif > Regarding the glibc headers, do you know why glibc doesn't just add code for > __clang__ rather than Clang (and ICC) claim to be a gcc compatible to a point > compiler? clang pretends to be gcc 4.2 because that was the simplest way to make a lot of existing code work, at the time clang was written. clang supports almost all the language extensions supported by that version of gcc, so code was more likely to work if clang pretended to be gcc, rather than some unknown compiler. And now, it would break a bunch of code if it changed, so it basically can't be changed. This is similar to the history of web browser user agent strings. For new code, we encourage users to use the feature checking macros (https://clang.llvm.org/docs/LanguageExtensions.html#feature-checking-macros ), rather than checking for specific compiler versions, where possible. Repository: rC Clang https://reviews.llvm.org/D51011 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46535: Correct warning on Float->Integer conversions.
efriedma added a comment.
The check for whether an input is "out of range" doesn't handle fractions
correctly. Testcase:
int a() { return 2147483647.5; }
unsigned b() { return -.5; }
Repository:
rC Clang
https://reviews.llvm.org/D46535
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46349: [X86] Mark builtins 'const' where possible
efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. I'm pretty sure this has zero visible effect, but I guess it makes sense as documentation. LGTM. https://reviews.llvm.org/D46349 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46112: Allow _Atomic to be specified on incomplete types
efriedma added a comment.
I think the request was that we check that a type is trivially copyable when we
perform an atomic operation? I don't see the code for that anywhere.
Also needs some test coverage for atomic operations which aren't calls, like
"typedef struct S S; void f(_Atomic S *s, _Atomic S *s2) { *s = *s2; };".
https://reviews.llvm.org/D46112
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45616: [X86] Lower _mm[256|512]_cmp[.]_mask intrinsics to native llvm IR
efriedma added a comment. There is no difference between "signalling" and "non-signalling" unless you're using "#pragma STDC FENV_ACCESS", which is currently not supported. Presumably the work to implement that will include some LLVM IR intrinsic which can encode the difference, but for now we can ignore it. Repository: rC Clang https://reviews.llvm.org/D45616 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46822: [RISCV] Add driver for riscv32-unknown-elf baremetal target
efriedma added a comment. Could you include some documentation for how to construct a baremetal environment like the one this code expects? It's not clear what exactly you expect to be installed where. Repository: rC Clang https://reviews.llvm.org/D46822 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46863: [X86] Use __builtin_convertvector to implement some of the packed integer to packed flow conversion intrinsics.
efriedma added a comment. > I'm concerned about constant folding not taking into account runtime rounding > mode Changing the rounding mode is UB without FENV_ACCESS. And with FENV_ACCESS, __builtin_convertvector should lower to @llvm.experimental.constrained.sitofp etc., which won't constant-fold. (llvm.experimental.constrained.sitofp doesn't actually exist yet, but I assume it will eventually get added.) Repository: rC Clang https://reviews.llvm.org/D46863 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46822: [RISCV] Add driver for riscv32-unknown-elf baremetal target
efriedma added a comment. Just a brief comment in the code explaining that would be fine. Repository: rC Clang https://reviews.llvm.org/D46822 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46665: [Itanium] Emit type info names with external linkage.
efriedma added a comment. The only difference between weak_odr and linkonce_odr is that the LLVM optimizers can discard linkonce_odr globals. From your description, you want to remove the odr-ness, by changing the linkage to "linkonce", I think? That said, I don't think the usage here violates LangRef's definition of linkonce_odr; the "odr"-ness refers to the global's initializer, not its linkage. Repository: rC Clang https://reviews.llvm.org/D46665 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46665: [Itanium] Emit type info names with external linkage.
efriedma added a comment. What exactly is the asan odr checker actually checking for? The distinction between common/linkonce_odr/linkonce/weak_odr/weak only affects IR optimizers, not code generation. Repository: rC Clang https://reviews.llvm.org/D46665 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47092: downgrade strong type info names to weak_odr linkage
efriedma added a comment. > This results in formal violations of LLVM's linkage rules I'm pretty sure this isn't actually a violation of LLVM's linkage rules as they are described in LangRef... at least, my understanding is that "equivalent" doesn't imply anything about linkage. (If this should be a violation, we need to clarify the rules.) Repository: rC Clang https://reviews.llvm.org/D47092 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45616: [X86] Lower _mm[256|512]_cmp[.]_mask intrinsics to native llvm IR
efriedma added a subscriber: andrew.w.kaylor. efriedma added a comment. I don't see any reason to exactly match the constant specified by the user, as long as the result is semantically equivalent. Once we have llvm.experimental.constrained.fcmp, this code should be updated to emit it; that will preserve the user's intended exception semantics. Repository: rC Clang https://reviews.llvm.org/D45616 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D36487: Emit section information for extern variables.
efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. LGTM https://reviews.llvm.org/D36487 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37861: preserving #pragma clang assume_nonnull in preprocessed output
efriedma added a comment. Oops, sorry, lost track of it; I'll commit it today. https://reviews.llvm.org/D37861 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37861: preserving #pragma clang assume_nonnull in preprocessed output
This revision was automatically updated to reflect the committed changes.
Closed by commit rL314364: [Preprocessor] Preserve #pragma clang assume_nonnull
in preprocessed output (authored by efriedma).
Changed prior to commit:
https://reviews.llvm.org/D37861?vs=115838&id=116903#toc
Repository:
rL LLVM
https://reviews.llvm.org/D37861
Files:
cfe/trunk/include/clang/Lex/PPCallbacks.h
cfe/trunk/lib/Frontend/PrintPreprocessedOutput.cpp
cfe/trunk/lib/Lex/Pragma.cpp
cfe/trunk/test/Preprocessor/pragma_assume_nonnull.c
Index: cfe/trunk/lib/Lex/Pragma.cpp
===
--- cfe/trunk/lib/Lex/Pragma.cpp
+++ cfe/trunk/lib/Lex/Pragma.cpp
@@ -1725,21 +1725,26 @@
// The start location we want after processing this.
SourceLocation NewLoc;
+PPCallbacks *Callbacks = PP.getPPCallbacks();
if (IsBegin) {
// Complain about attempts to re-enter an audit.
if (BeginLoc.isValid()) {
PP.Diag(Loc, diag::err_pp_double_begin_of_assume_nonnull);
PP.Diag(BeginLoc, diag::note_pragma_entered_here);
}
NewLoc = Loc;
+ if (Callbacks)
+Callbacks->PragmaAssumeNonNullBegin(NewLoc);
} else {
// Complain about attempts to leave an audit that doesn't exist.
if (!BeginLoc.isValid()) {
PP.Diag(Loc, diag::err_pp_unmatched_end_of_assume_nonnull);
return;
}
NewLoc = SourceLocation();
+ if (Callbacks)
+Callbacks->PragmaAssumeNonNullEnd(NewLoc);
}
PP.setPragmaAssumeNonNullLoc(NewLoc);
Index: cfe/trunk/lib/Frontend/PrintPreprocessedOutput.cpp
===
--- cfe/trunk/lib/Frontend/PrintPreprocessedOutput.cpp
+++ cfe/trunk/lib/Frontend/PrintPreprocessedOutput.cpp
@@ -143,6 +143,8 @@
ArrayRef Ids) override;
void PragmaWarningPush(SourceLocation Loc, int Level) override;
void PragmaWarningPop(SourceLocation Loc) override;
+ void PragmaAssumeNonNullBegin(SourceLocation Loc) override;
+ void PragmaAssumeNonNullEnd(SourceLocation Loc) override;
bool HandleFirstTokOnLine(Token &Tok);
@@ -549,6 +551,22 @@
setEmittedDirectiveOnThisLine();
}
+void PrintPPOutputPPCallbacks::
+PragmaAssumeNonNullBegin(SourceLocation Loc) {
+ startNewLineIfNeeded();
+ MoveToLine(Loc);
+ OS << "#pragma clang assume_nonnull begin";
+ setEmittedDirectiveOnThisLine();
+}
+
+void PrintPPOutputPPCallbacks::
+PragmaAssumeNonNullEnd(SourceLocation Loc) {
+ startNewLineIfNeeded();
+ MoveToLine(Loc);
+ OS << "#pragma clang assume_nonnull end";
+ setEmittedDirectiveOnThisLine();
+}
+
/// HandleFirstTokOnLine - When emitting a preprocessed file in -E mode, this
/// is called for the first token on each new line. If this really is the start
/// of a new logical line, handle it and return true, otherwise return false.
Index: cfe/trunk/include/clang/Lex/PPCallbacks.h
===
--- cfe/trunk/include/clang/Lex/PPCallbacks.h
+++ cfe/trunk/include/clang/Lex/PPCallbacks.h
@@ -235,6 +235,14 @@
virtual void PragmaWarningPop(SourceLocation Loc) {
}
+ /// \brief Callback invoked when a \#pragma clang assume_nonnull begin directive
+ /// is read.
+ virtual void PragmaAssumeNonNullBegin(SourceLocation Loc) {}
+
+ /// \brief Callback invoked when a \#pragma clang assume_nonnull end directive
+ /// is read.
+ virtual void PragmaAssumeNonNullEnd(SourceLocation Loc) {}
+
/// \brief Called by Preprocessor::HandleMacroExpandedIdentifier when a
/// macro invocation is found.
virtual void MacroExpands(const Token &MacroNameTok,
@@ -446,6 +454,16 @@
Second->PragmaWarningPop(Loc);
}
+ void PragmaAssumeNonNullBegin(SourceLocation Loc) override {
+First->PragmaAssumeNonNullBegin(Loc);
+Second->PragmaAssumeNonNullBegin(Loc);
+ }
+
+ void PragmaAssumeNonNullEnd(SourceLocation Loc) override {
+First->PragmaAssumeNonNullEnd(Loc);
+Second->PragmaAssumeNonNullEnd(Loc);
+ }
+
void MacroExpands(const Token &MacroNameTok, const MacroDefinition &MD,
SourceRange Range, const MacroArgs *Args) override {
First->MacroExpands(MacroNameTok, MD, Range, Args);
Index: cfe/trunk/test/Preprocessor/pragma_assume_nonnull.c
===
--- cfe/trunk/test/Preprocessor/pragma_assume_nonnull.c
+++ cfe/trunk/test/Preprocessor/pragma_assume_nonnull.c
@@ -0,0 +1,16 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+// RUN: %clang_cc1 -E %s | FileCheck %s
+
+// CHECK: #pragma clang assume_nonnull begin
+#pragma clang assume_nonnull begin
+
+int bar(int * ip) { return *ip; }
+
+// CHECK: #pragma clang assume_nonnull end
+#pragma clang assume_nonnull end
+
+int foo(int * _Nonnull ip) { return *ip; }
+
+int main() {
+ return bar(0) + foo(0); // expected-warning 2 {{null passed to a callee that requires a non-null argument}}
+}
[PATCH] D38479: Make -mgeneral-regs-only more like GCC's
efriedma added a comment. As far as I can see, there are three significant issues with the current -mgeneral-regs-only: 1. We don't correctly ignore inline asm clobbers for registers which aren't allocatable (https://bugs.llvm.org/show_bug.cgi?id=30792) 2. We don't diagnose calls which need vector registers according to the C calling convention. 3. We don't diagnose operations which have to be lowered to libcalls which need vector registers according to the C calling convention (fptosi, @llvm.sin.*, etc.). All three of these could be addressesed in the AArch64 backend in a straightforward manner. Diagnosing floating-point operations in Sema in addition to whatever backend fixes we might want is fine, I guess, but I don't really like making "-mgeneral-regs-only" into "-mno-implicit-float" plus some diagnostics; other frontends don't benefit from this checking, and using no-implicit-float is asking for an obscure miscompile if IR generation or an optimization accidentally produces a floating-point value. https://reviews.llvm.org/D38479 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38717: Patch to Bugzilla 31373
efriedma added a comment. Please make sure the title (subject line) for a patch reflects the actual contents of the change, as opposed to referring to Bugzilla; a lot of people skim cfe-commits, so you want to make sure interested reviewers find you patch. Needs a regression test to verify we generate the warning in cases we should, and don't generate the warning in cases where we shouldn't (probably want a few tests with variables whose type is a class with a non-trivial destructor, or a reference to a temporary with a non-trivial destructor; removing the variable could change the behavior in those cases). There are plenty of examples to follow in "test/SemaCXX/" in the source. I don't really like messing with the value of the "Used" bit; isUsed() is supposed to reflect whether a variable is odr-used, and getting it wrong can have weird implications for the way we type-check and emit code. (Most of those implications don't really apply to local variables, but it's still confusing.) isReferenced() is our "is this declaration doing anything useful" bit. Can we somehow change the way we set and use the "Referenced" bit to come up with the right result here? https://reviews.llvm.org/D38717 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D39122: [Sema] Fixes for enum handling for tautological comparison diagnostics
efriedma added inline comments. Comment at: lib/Sema/SemaChecking.cpp:8612 +OtherT = OtherT->getAs()->getDecl()->getIntegerType(); + } + Why are you changing this here, as opposed to changing IntRange::forValueOfCanonicalType? Repository: rL LLVM https://reviews.llvm.org/D39122 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D39204: [CodeGen] __builtin_sqrt should map to the compiler's intrinsic sqrt function
efriedma added a comment. The gcc documentation says "GCC includes built-in versions of many of the functions in the standard C library. These functions come in two forms: one whose names start with the `__builtin_` prefix, and the other without. Both forms have the same type (including prototype), the same address (when their address is taken), and the same meaning as the C library functions". And gcc specifically preserves the stated semantics. Given that, I'm not sure it makes sense for us to try to redefine `__builtin_sqrt()` just because it's convenient. Note that this reasoning only applies if the user hasn't specified any fast-math flags; under -ffinite-math-only, we can assume the result isn't a NaN, and therefore we can use `llvm.sqrt.*`. (The definition of `llvm.sqrt.*` changed in https://reviews.llvm.org/D28797; I don't think we ever updated clang to take advantage of this). If we really need a name for the never-sets-errno sqrt, we should probably use a different name, e.g. `__builtin_ieee_sqrt()`. https://reviews.llvm.org/D39204 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D39204: [CodeGen] __builtin_sqrt should map to the compiler's intrinsic sqrt function
efriedma added a comment. I think you're understanding the semantics correctly. For r265521, look again at the implementation of llvm::checkUnaryFloatSignature; if "I.onlyReadsMemory()" is true, we somehow proved the call doesn't set errno (mostly likely because we know something about the target's libm). https://reviews.llvm.org/D39204 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51198: [LTO] Fix -save-temps with LTO and unnamed globals.
efriedma created this revision.
efriedma added reviewers: tejohnson, tobiasvk.
Herald added subscribers: dexonsmith, inglorion.
If all LLVM passes are disabled, we can't emit a summary because there could be
unnamed globals in the IR.
Repository:
rC Clang
https://reviews.llvm.org/D51198
Files:
lib/CodeGen/BackendUtil.cpp
test/CodeGen/summary-index-unnamed-global.ll
Index: test/CodeGen/summary-index-unnamed-global.ll
===
--- /dev/null
+++ test/CodeGen/summary-index-unnamed-global.ll
@@ -0,0 +1,8 @@
+; RUN: %clang_cc1 -flto -triple x86_64-pc-linux-gnu -emit-llvm-bc
-disable-llvm-passes -x ir < %s -o - | llvm-bcanalyzer -dump | FileCheck %s
+; RUN: %clang_cc1 -flto=thin -triple x86_64-pc-linux-gnu -emit-llvm-bc
-disable-llvm-passes -x ir < %s -o - | llvm-bcanalyzer -dump | FileCheck %s
+
+; CHECK-NOT:GLOBALVAL_SUMMARY_BLOCK
+
+; Make sure this doesn't crash, and we don't try to emit a module summary.
+; (The command is roughly emulating what -save-temps would do.)
+@0 = global i32 0
Index: lib/CodeGen/BackendUtil.cpp
===
--- lib/CodeGen/BackendUtil.cpp
+++ lib/CodeGen/BackendUtil.cpp
@@ -783,7 +783,7 @@
break;
case Backend_EmitBC:
-if (CodeGenOpts.PrepareForThinLTO) {
+if (CodeGenOpts.PrepareForThinLTO && !CodeGenOpts.DisableLLVMPasses) {
if (!CodeGenOpts.ThinLinkBitcodeFile.empty()) {
ThinLinkOS = openOutputFile(CodeGenOpts.ThinLinkBitcodeFile);
if (!ThinLinkOS)
@@ -796,6 +796,7 @@
// targets
bool EmitLTOSummary =
(CodeGenOpts.PrepareForLTO &&
+ !CodeGenOpts.DisableLLVMPasses &&
llvm::Triple(TheModule->getTargetTriple()).getVendor() !=
llvm::Triple::Apple);
if (EmitLTOSummary && !TheModule->getModuleFlag("ThinLTO"))
Index: test/CodeGen/summary-index-unnamed-global.ll
===
--- /dev/null
+++ test/CodeGen/summary-index-unnamed-global.ll
@@ -0,0 +1,8 @@
+; RUN: %clang_cc1 -flto -triple x86_64-pc-linux-gnu -emit-llvm-bc -disable-llvm-passes -x ir < %s -o - | llvm-bcanalyzer -dump | FileCheck %s
+; RUN: %clang_cc1 -flto=thin -triple x86_64-pc-linux-gnu -emit-llvm-bc -disable-llvm-passes -x ir < %s -o - | llvm-bcanalyzer -dump | FileCheck %s
+
+; CHECK-NOT:GLOBALVAL_SUMMARY_BLOCK
+
+; Make sure this doesn't crash, and we don't try to emit a module summary.
+; (The command is roughly emulating what -save-temps would do.)
+@0 = global i32 0
Index: lib/CodeGen/BackendUtil.cpp
===
--- lib/CodeGen/BackendUtil.cpp
+++ lib/CodeGen/BackendUtil.cpp
@@ -783,7 +783,7 @@
break;
case Backend_EmitBC:
-if (CodeGenOpts.PrepareForThinLTO) {
+if (CodeGenOpts.PrepareForThinLTO && !CodeGenOpts.DisableLLVMPasses) {
if (!CodeGenOpts.ThinLinkBitcodeFile.empty()) {
ThinLinkOS = openOutputFile(CodeGenOpts.ThinLinkBitcodeFile);
if (!ThinLinkOS)
@@ -796,6 +796,7 @@
// targets
bool EmitLTOSummary =
(CodeGenOpts.PrepareForLTO &&
+ !CodeGenOpts.DisableLLVMPasses &&
llvm::Triple(TheModule->getTargetTriple()).getVendor() !=
llvm::Triple::Apple);
if (EmitLTOSummary && !TheModule->getModuleFlag("ThinLTO"))
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51198: [LTO] Fix -save-temps with LTO and unnamed globals.
efriedma updated this revision to Diff 162308.
efriedma added a comment.
Fix new pass manager.
Repository:
rC Clang
https://reviews.llvm.org/D51198
Files:
lib/CodeGen/BackendUtil.cpp
test/CodeGen/summary-index-unnamed-global.ll
Index: test/CodeGen/summary-index-unnamed-global.ll
===
--- /dev/null
+++ test/CodeGen/summary-index-unnamed-global.ll
@@ -0,0 +1,10 @@
+; RUN: %clang_cc1 -flto -triple x86_64-pc-linux-gnu -emit-llvm-bc
-disable-llvm-passes -x ir < %s -o - | llvm-bcanalyzer -dump | FileCheck %s
+; RUN: %clang_cc1 -flto=thin -triple x86_64-pc-linux-gnu -emit-llvm-bc
-disable-llvm-passes -x ir < %s -o - | llvm-bcanalyzer -dump | FileCheck %s
+; RUN: %clang_cc1 -fexperimental-new-pass-manager -flto -triple
x86_64-pc-linux-gnu -emit-llvm-bc -disable-llvm-passes -x ir < %s -o - |
llvm-bcanalyzer -dump | FileCheck %s
+; RUN: %clang_cc1 -fexperimental-new-pass-manager -flto=thin -triple
x86_64-pc-linux-gnu -emit-llvm-bc -disable-llvm-passes -x ir < %s -o - |
llvm-bcanalyzer -dump | FileCheck %s
+
+; CHECK-NOT:GLOBALVAL_SUMMARY_BLOCK
+
+; Make sure this doesn't crash, and we don't try to emit a module summary.
+; (The command is roughly emulating what -save-temps would do.)
+@0 = global i32 0
Index: lib/CodeGen/BackendUtil.cpp
===
--- lib/CodeGen/BackendUtil.cpp
+++ lib/CodeGen/BackendUtil.cpp
@@ -783,7 +783,7 @@
break;
case Backend_EmitBC:
-if (CodeGenOpts.PrepareForThinLTO) {
+if (CodeGenOpts.PrepareForThinLTO && !CodeGenOpts.DisableLLVMPasses) {
if (!CodeGenOpts.ThinLinkBitcodeFile.empty()) {
ThinLinkOS = openOutputFile(CodeGenOpts.ThinLinkBitcodeFile);
if (!ThinLinkOS)
@@ -796,6 +796,7 @@
// targets
bool EmitLTOSummary =
(CodeGenOpts.PrepareForLTO &&
+ !CodeGenOpts.DisableLLVMPasses &&
llvm::Triple(TheModule->getTargetTriple()).getVendor() !=
llvm::Triple::Apple);
if (EmitLTOSummary && !TheModule->getModuleFlag("ThinLTO"))
@@ -1014,7 +1015,7 @@
break;
case Backend_EmitBC:
-if (CodeGenOpts.PrepareForThinLTO) {
+if (CodeGenOpts.PrepareForThinLTO && !CodeGenOpts.DisableLLVMPasses) {
if (!CodeGenOpts.ThinLinkBitcodeFile.empty()) {
ThinLinkOS = openOutputFile(CodeGenOpts.ThinLinkBitcodeFile);
if (!ThinLinkOS)
@@ -1027,6 +1028,7 @@
// targets
bool EmitLTOSummary =
(CodeGenOpts.PrepareForLTO &&
+ !CodeGenOpts.DisableLLVMPasses &&
llvm::Triple(TheModule->getTargetTriple()).getVendor() !=
llvm::Triple::Apple);
if (EmitLTOSummary && !TheModule->getModuleFlag("ThinLTO"))
Index: test/CodeGen/summary-index-unnamed-global.ll
===
--- /dev/null
+++ test/CodeGen/summary-index-unnamed-global.ll
@@ -0,0 +1,10 @@
+; RUN: %clang_cc1 -flto -triple x86_64-pc-linux-gnu -emit-llvm-bc -disable-llvm-passes -x ir < %s -o - | llvm-bcanalyzer -dump | FileCheck %s
+; RUN: %clang_cc1 -flto=thin -triple x86_64-pc-linux-gnu -emit-llvm-bc -disable-llvm-passes -x ir < %s -o - | llvm-bcanalyzer -dump | FileCheck %s
+; RUN: %clang_cc1 -fexperimental-new-pass-manager -flto -triple x86_64-pc-linux-gnu -emit-llvm-bc -disable-llvm-passes -x ir < %s -o - | llvm-bcanalyzer -dump | FileCheck %s
+; RUN: %clang_cc1 -fexperimental-new-pass-manager -flto=thin -triple x86_64-pc-linux-gnu -emit-llvm-bc -disable-llvm-passes -x ir < %s -o - | llvm-bcanalyzer -dump | FileCheck %s
+
+; CHECK-NOT:GLOBALVAL_SUMMARY_BLOCK
+
+; Make sure this doesn't crash, and we don't try to emit a module summary.
+; (The command is roughly emulating what -save-temps would do.)
+@0 = global i32 0
Index: lib/CodeGen/BackendUtil.cpp
===
--- lib/CodeGen/BackendUtil.cpp
+++ lib/CodeGen/BackendUtil.cpp
@@ -783,7 +783,7 @@
break;
case Backend_EmitBC:
-if (CodeGenOpts.PrepareForThinLTO) {
+if (CodeGenOpts.PrepareForThinLTO && !CodeGenOpts.DisableLLVMPasses) {
if (!CodeGenOpts.ThinLinkBitcodeFile.empty()) {
ThinLinkOS = openOutputFile(CodeGenOpts.ThinLinkBitcodeFile);
if (!ThinLinkOS)
@@ -796,6 +796,7 @@
// targets
bool EmitLTOSummary =
(CodeGenOpts.PrepareForLTO &&
+ !CodeGenOpts.DisableLLVMPasses &&
llvm::Triple(TheModule->getTargetTriple()).getVendor() !=
llvm::Triple::Apple);
if (EmitLTOSummary && !TheModule->getModuleFlag("ThinLTO"))
@@ -1014,7 +1015,7 @@
break;
case Backend_EmitBC:
-if (CodeGenOpts.PrepareForThinLTO) {
+if (CodeGenOpts.PrepareForThinLTO && !CodeGenOpts.DisableLLVMPasses) {
if (!CodeGenOpts.ThinLinkBitcodeFile.empty()) {
ThinLinkOS = openOutputFile(CodeGenOpts.ThinLinkBitcodeFile);
if (!ThinLinkOS)
@@ -1027,6 +1028,
[PATCH] D51198: [LTO] Fix -save-temps with LTO and unnamed globals.
This revision was automatically updated to reflect the committed changes.
Closed by commit rC340640: [LTO] Fix -save-temps with LTO and unnamed globals.
(authored by efriedma, committed by ).
Repository:
rC Clang
https://reviews.llvm.org/D51198
Files:
lib/CodeGen/BackendUtil.cpp
test/CodeGen/summary-index-unnamed-global.ll
Index: lib/CodeGen/BackendUtil.cpp
===
--- lib/CodeGen/BackendUtil.cpp
+++ lib/CodeGen/BackendUtil.cpp
@@ -783,7 +783,7 @@
break;
case Backend_EmitBC:
-if (CodeGenOpts.PrepareForThinLTO) {
+if (CodeGenOpts.PrepareForThinLTO && !CodeGenOpts.DisableLLVMPasses) {
if (!CodeGenOpts.ThinLinkBitcodeFile.empty()) {
ThinLinkOS = openOutputFile(CodeGenOpts.ThinLinkBitcodeFile);
if (!ThinLinkOS)
@@ -796,6 +796,7 @@
// targets
bool EmitLTOSummary =
(CodeGenOpts.PrepareForLTO &&
+ !CodeGenOpts.DisableLLVMPasses &&
llvm::Triple(TheModule->getTargetTriple()).getVendor() !=
llvm::Triple::Apple);
if (EmitLTOSummary && !TheModule->getModuleFlag("ThinLTO"))
@@ -1014,7 +1015,7 @@
break;
case Backend_EmitBC:
-if (CodeGenOpts.PrepareForThinLTO) {
+if (CodeGenOpts.PrepareForThinLTO && !CodeGenOpts.DisableLLVMPasses) {
if (!CodeGenOpts.ThinLinkBitcodeFile.empty()) {
ThinLinkOS = openOutputFile(CodeGenOpts.ThinLinkBitcodeFile);
if (!ThinLinkOS)
@@ -1027,6 +1028,7 @@
// targets
bool EmitLTOSummary =
(CodeGenOpts.PrepareForLTO &&
+ !CodeGenOpts.DisableLLVMPasses &&
llvm::Triple(TheModule->getTargetTriple()).getVendor() !=
llvm::Triple::Apple);
if (EmitLTOSummary && !TheModule->getModuleFlag("ThinLTO"))
Index: test/CodeGen/summary-index-unnamed-global.ll
===
--- test/CodeGen/summary-index-unnamed-global.ll
+++ test/CodeGen/summary-index-unnamed-global.ll
@@ -0,0 +1,10 @@
+; RUN: %clang_cc1 -flto -triple x86_64-pc-linux-gnu -emit-llvm-bc
-disable-llvm-passes -x ir < %s -o - | llvm-bcanalyzer -dump | FileCheck %s
+; RUN: %clang_cc1 -flto=thin -triple x86_64-pc-linux-gnu -emit-llvm-bc
-disable-llvm-passes -x ir < %s -o - | llvm-bcanalyzer -dump | FileCheck %s
+; RUN: %clang_cc1 -fexperimental-new-pass-manager -flto -triple
x86_64-pc-linux-gnu -emit-llvm-bc -disable-llvm-passes -x ir < %s -o - |
llvm-bcanalyzer -dump | FileCheck %s
+; RUN: %clang_cc1 -fexperimental-new-pass-manager -flto=thin -triple
x86_64-pc-linux-gnu -emit-llvm-bc -disable-llvm-passes -x ir < %s -o - |
llvm-bcanalyzer -dump | FileCheck %s
+
+; CHECK-NOT:GLOBALVAL_SUMMARY_BLOCK
+
+; Make sure this doesn't crash, and we don't try to emit a module summary.
+; (The command is roughly emulating what -save-temps would do.)
+@0 = global i32 0
Index: lib/CodeGen/BackendUtil.cpp
===
--- lib/CodeGen/BackendUtil.cpp
+++ lib/CodeGen/BackendUtil.cpp
@@ -783,7 +783,7 @@
break;
case Backend_EmitBC:
-if (CodeGenOpts.PrepareForThinLTO) {
+if (CodeGenOpts.PrepareForThinLTO && !CodeGenOpts.DisableLLVMPasses) {
if (!CodeGenOpts.ThinLinkBitcodeFile.empty()) {
ThinLinkOS = openOutputFile(CodeGenOpts.ThinLinkBitcodeFile);
if (!ThinLinkOS)
@@ -796,6 +796,7 @@
// targets
bool EmitLTOSummary =
(CodeGenOpts.PrepareForLTO &&
+ !CodeGenOpts.DisableLLVMPasses &&
llvm::Triple(TheModule->getTargetTriple()).getVendor() !=
llvm::Triple::Apple);
if (EmitLTOSummary && !TheModule->getModuleFlag("ThinLTO"))
@@ -1014,7 +1015,7 @@
break;
case Backend_EmitBC:
-if (CodeGenOpts.PrepareForThinLTO) {
+if (CodeGenOpts.PrepareForThinLTO && !CodeGenOpts.DisableLLVMPasses) {
if (!CodeGenOpts.ThinLinkBitcodeFile.empty()) {
ThinLinkOS = openOutputFile(CodeGenOpts.ThinLinkBitcodeFile);
if (!ThinLinkOS)
@@ -1027,6 +1028,7 @@
// targets
bool EmitLTOSummary =
(CodeGenOpts.PrepareForLTO &&
+ !CodeGenOpts.DisableLLVMPasses &&
llvm::Triple(TheModule->getTargetTriple()).getVendor() !=
llvm::Triple::Apple);
if (EmitLTOSummary && !TheModule->getModuleFlag("ThinLTO"))
Index: test/CodeGen/summary-index-unnamed-global.ll
===
--- test/CodeGen/summary-index-unnamed-global.ll
+++ test/CodeGen/summary-index-unnamed-global.ll
@@ -0,0 +1,10 @@
+; RUN: %clang_cc1 -flto -triple x86_64-pc-linux-gnu -emit-llvm-bc -disable-llvm-passes -x ir < %s -o - | llvm-bcanalyzer -dump | FileCheck %s
+; RUN: %clang_cc1 -flto=thin -triple x86_64-pc-linux-gnu -emit-llvm-bc -disable-llvm-passes -x ir < %s -o - | llvm-bcanalyzer -dump | FileCheck %s
+; RUN: %clang_cc1 -fexperimental-new-pass-manager -flto -triple x86_64-pc-l
[PATCH] D51265: Headers: fix collisions with .h files of other projects
efriedma edited reviewers, added: efriedma, rsmith, thakis; removed: eli.friedman, krememek, ddunbar, lattner. efriedma added a comment. What is this change actually solving? Even if the typedef is actually redundant, it shouldn't cause a compiler error: redundant typedefs are allowed in the latest versions of the C and C++ standards. (And even if you were targeting an earlier standard, we normally suppress warnings in system headers.) Repository: rC Clang https://reviews.llvm.org/D51265 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51190: [AttrDocs]: document gnu_inline function attribute
efriedma added a comment. Thanks for taking the time to write documentation. The phrase "C89 convention" is misleading; the original ISO C standard doesn't define the inline keyword at all. Maybe something along the lines of "GNU inline extension". And maybe mention it's the default with -std=gnu89. Repository: rC Clang https://reviews.llvm.org/D51190 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51265: Headers: fix collisions with .h files of other projects
efriedma added a comment. Can you give the text of the error messages? If you're seeing a "typedef redefinition with different types" error, libclang is getting confused about the target. And parsing code for the wrong target cannot work in general. Repository: rC Clang https://reviews.llvm.org/D51265 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51416: [RTTI] Align rtti type string to prevent over-alignment
efriedma added a comment. Could we make CreateOrReplaceCXXRuntimeVariable take the alignment as an argument, so we can be sure we're consistently setting the alignment for these variables? This seems easy to mess up if we're scattering calls all over the place. Sort of orthogonal, but CreateOrReplaceCXXRuntimeVariable should probably also call setUnnamedAddr, instead of making each caller call it. https://reviews.llvm.org/D51416 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50229: +fp16fml feature for ARM and AArch64
efriedma added a comment. Do you expect that the regression tests will be affected by the TargetParser fixes? If not, I guess this is okay... but please add clear comments explaining which bits of code you expect to go away after the TargetParser rewrite. Repository: rC Clang https://reviews.llvm.org/D50229 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51416: [RTTI] Align rtti types to prevent over-alignment
efriedma added a comment. getClassPointerAlignment is not really relevant here; that's the alignment of a pointer to an object with the type of the class. For the LLVM IR structs which don't have a corresponding clang type, you can probably just use DataLayout::getABITypeAlignment(). I think that's just the alignment of a pointer in practice, but the intent is more clear. Comment at: lib/CodeGen/MicrosoftCXXABI.cpp:2027 "vbtable with this name already exists: mangling bug?"); + unsigned Align = CGM.getClassPointerAlignment(RD).getQuantity(); llvm::GlobalVariable *GV = This is an array of `int`. https://reviews.llvm.org/D51416 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51817: Do not use optimized atomic libcalls for misaligned atomics.
efriedma added inline comments. Comment at: lib/CodeGen/CGAtomic.cpp:949 case AtomicExpr::AO__opencl_atomic_compare_exchange_strong: case AtomicExpr::AO__atomic_load_n: case AtomicExpr::AO__atomic_store_n: Is there any particular reason to expect that the pointer operand to __atomic_load_n can't be misaligned? I mean, for most ABIs, integers are naturally aligned, but that isn't actually a hard rule. Repository: rC Clang https://reviews.llvm.org/D51817 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51817: Do not use optimized atomic libcalls for misaligned atomics.
efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. LGTM Comment at: lib/CodeGen/CGAtomic.cpp:949 case AtomicExpr::AO__opencl_atomic_compare_exchange_strong: case AtomicExpr::AO__atomic_load_n: case AtomicExpr::AO__atomic_store_n: rsmith wrote: > efriedma wrote: > > Is there any particular reason to expect that the pointer operand to > > __atomic_load_n can't be misaligned? I mean, for most ABIs, integers are > > naturally aligned, but that isn't actually a hard rule. > `__atomic_load_n` is, by definition, guaranteed to never call an unoptimized > atomic library function (see https://gcc.gnu.org/wiki/Atomic/GCCMM/LIbrary). > [I think the purpose of the `..._n` variants is to provide builtins that > libatomic's unoptimized library functions can use and have a guarantee that > they will not be recursively re-entered.] Oh, okay, makes sense. Repository: rC Clang https://reviews.llvm.org/D51817 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51416: [RTTI] Align rtti types to prevent over-alignment
efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. LGTM with one change. Comment at: lib/CodeGen/ItaniumCXXABI.cpp:2730 llvm::GlobalVariable *GV = -CGM.CreateOrReplaceCXXRuntimeVariable(Name, Init->getType(), Linkage); + CGM.CreateOrReplaceCXXRuntimeVariable(Name, Init->getType(), Linkage, 1); Please use getTypeAlignInChars(getContext().CharTy), or something like that, so it's clear where the "1" is coming from. https://reviews.llvm.org/D51416 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50507: [CodeGen][ARM] Coerce FP16 vectors to integer vectors when needed
efriedma accepted this revision. efriedma added a comment. LGTM https://reviews.llvm.org/D50507 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46013: [ARM] Conform to AAPCS when passing overaligned composites as arguments
efriedma added inline comments. Comment at: lib/AST/ASTContext.cpp:2088 + default: +UnadjustedAlign = getTypeAlign(T); + } This "default" isn't right; there are a lot of non-canonical types which need to be handled here (which getTypeAlign handles, but this doesn't). Could you just write something like `if (const auto *RTy = T->getAs()) [...] else return getTypeAlign(T);`? https://reviews.llvm.org/D46013 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45712: Diagnose invalid cv-qualifiers for friend decls.
efriedma added a comment. Herald added a subscriber: jfb. Ping Repository: rC Clang https://reviews.llvm.org/D45712 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46013: [ARM] Conform to AAPCS when passing overaligned composites as arguments
efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. LGTM https://reviews.llvm.org/D46013 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47196: [Time-report ](2): Recursive timers in Clang
efriedma added inline comments.
Comment at: lib/Sema/SemaLambda.cpp:1447
+getFrontendFunctionTimeCtx()->startFrontendTimer(
+{LSI.CallOperator, 0.0});
+ }
This seems sort of late? You're starting the timer after the body has already
been parsed.
Comment at: lib/Sema/TreeTransform.h:11011
+getFrontendFunctionTimeCtx()->startFrontendTimer(
+{NewCallOperator, 0.0});
+ }
What happens if we never hit ActOnFinishFunctionBody()? TransformLambdaExpr
has an early return if the body doesn't typecheck.
More generally, given that we have early returns all over the place in Sema, I
would be more comfortable using the RAII helper, rather than explicitly calling
start/stop, even if that means you have to insert FrontendTimeRAII variables in
half a dozen different places in the parser. (Note I'm specifically talking
about the parser here; the explicit stopFrontendTimer in ~CodeGenFunction seems
fine.)
https://reviews.llvm.org/D47196
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46822: [RISCV] Add driver for riscv32-unknown-elf baremetal target
efriedma added a comment. Please don't commit patches on behalf of someone else if the author hasn't specifically requested it. There might be some issue which the author forgot to note on the review. Repository: rC Clang https://reviews.llvm.org/D46822 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47196: [Time-report ](2): Recursive timers in Clang
efriedma added a comment.
I think there's something weird happening with your timers if they're showing
half a second spent parsing or instantiating std::declval: it doesn't have a
definition, anywhere (it's just a placeholder for template metaprogramming
tricks).
Comment at: lib/AST/Expr.cpp:544
+FrontendTimesIsEnabled,
+getFrontendFunctionTimeCtx(), {FD, 0});
if (IT != PrettyFunction && IT != PrettyFunctionNoVirtual &&
You shouldn't need a separate timer here; `__func__` should only show up inside
a function definition anyway.
Comment at: lib/Analysis/AnalysisDeclContext.cpp:99
+FrontendTimesIsEnabled,
+getFrontendFunctionTimeCtx(), {FD, 0});
Stmt *Body = FD->getBody();
Not sure what you're trying to time here.
Comment at: lib/Parse/Parser.cpp:1131
Actions.CheckForFunctionRedefinition(FnD);
Actions.MarkAsLateParsedTemplate(FnD, DP, Toks);
}
CheckForFunctionRedefinition and MarkAsLateParsedTemplate are both pretty
cheap; not sure it's worth separately timing them.
Comment at: lib/Sema/SemaDecl.cpp:2996
/// Returns true if there was an error, false otherwise.
bool Sema::MergeFunctionDecl(FunctionDecl *New, NamedDecl *&OldD,
Scope *S, bool MergeTypeWithOld) {
MergeFunctionDecl only has one caller: CheckFunctionDeclaration. Probably
makes more sense to time that, instead?
Comment at: lib/Sema/SemaDecl.cpp:3595
bool Sema::MergeCompatibleFunctionDecls(FunctionDecl *New, FunctionDecl *Old,
Scope *S, bool MergeTypeWithOld) {
+ FrontendTimeRAII FTRAII(
MergeCompatibleFunctionDecls is only called by MergeFunctionDecl.
https://reviews.llvm.org/D47196
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50179: [AArch64][ARM] Context sensitive meaning of option "crypto"
efriedma added inline comments.
Comment at: lib/Driver/ToolChains/Arch/AArch64.cpp:200
+ //
+ // TODO: implement this logic in TargetParser.
+
Yes, this logic should be in TargetParser, not here. Trying to rewrite the
target features afterwards is messy at best. (Actually, the target feature
list generated by TargetParser probably shouldn't contain the string "crypto"
at all.)
Given that this code will go away when TargetParser is fixed, why are you
proposing this patch?
Comment at: lib/Driver/ToolChains/Arch/AArch64.cpp:219
+
+ if (std::find(ItBegin, ItEnd, "+v8.4a") != ItEnd) {
+if (HasCrypto && !NoCrypto) {
Does this need to check for 8.5a?
Comment at: lib/Driver/ToolChains/Arch/ARM.cpp:430
+ if (ArchName.find_lower("+noaes") == StringRef::npos)
+Features.push_back("+aes");
+} else if (ArchName.find_lower("-crypto") != StringRef::npos) {
The ARM backend doesn't support features named "sha2" and "aes" at the moment.
https://reviews.llvm.org/D50179
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47196: [Time-report ](2): Recursive timers in Clang
efriedma added a comment. "0.0040" is four milliseconds? You're probably crediting time incorrectly, somehow. Can you tell which FrontendTimeRAII the time is coming from? https://reviews.llvm.org/D47196 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45712: Diagnose invalid cv-qualifiers for friend decls.
This revision was automatically updated to reflect the committed changes.
Closed by commit rL338931: Diagnose invalid cv-qualifiers for friend decls.
(authored by efriedma, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D45712?vs=154707&id=159103#toc
Repository:
rL LLVM
https://reviews.llvm.org/D45712
Files:
cfe/trunk/lib/Sema/SemaDeclCXX.cpp
cfe/trunk/test/CXX/class.access/class.friend/p3-cxx0x.cpp
cfe/trunk/test/Modules/odr_hash.cpp
Index: cfe/trunk/test/Modules/odr_hash.cpp
===
--- cfe/trunk/test/Modules/odr_hash.cpp
+++ cfe/trunk/test/Modules/odr_hash.cpp
@@ -2244,22 +2244,6 @@
#endif
#if defined(FIRST)
-struct T3 {};
-struct S3 {
- friend const T3;
-};
-#elif defined(SECOND)
-struct T3 {};
-struct S3 {
- friend T3;
-};
-#else
-S3 s3;
-// expected-error@second.h:* {{'Friend::S3' has different definitions in different modules; first difference is definition in module 'SecondModule' found friend 'Friend::T3'}}
-// expected-note@first.h:* {{but in 'FirstModule' found friend 'const Friend::T3'}}
-#endif
-
-#if defined(FIRST)
struct T4 {};
struct S4 {
friend T4;
@@ -2292,14 +2276,12 @@
friend class FriendA; \
friend struct FriendB; \
friend FriendC;\
- friend const FriendD; \
friend void Function();
#if defined(FIRST) || defined(SECOND)
class FriendA {};
class FriendB {};
class FriendC {};
-class FriendD {};
#endif
#if defined(FIRST) || defined(SECOND)
Index: cfe/trunk/test/CXX/class.access/class.friend/p3-cxx0x.cpp
===
--- cfe/trunk/test/CXX/class.access/class.friend/p3-cxx0x.cpp
+++ cfe/trunk/test/CXX/class.access/class.friend/p3-cxx0x.cpp
@@ -52,14 +52,25 @@
// Ill-formed
int friend; // expected-error {{'friend' must appear first in a non-function declaration}}
unsigned friend int; // expected-error {{'friend' must appear first in a non-function declaration}}
- const volatile friend int; // expected-error {{'friend' must appear first in a non-function declaration}}
+ const volatile friend int; // expected-error {{'friend' must appear first in a non-function declaration}} \
+ // expected-error {{'const' is invalid in friend declarations}} \
+ // expected-error {{'volatile' is invalid in friend declarations}}
int
friend; // expected-error {{'friend' must appear first in a non-function declaration}}
+ friend const int; // expected-error {{'const' is invalid in friend declarations}}
+ friend volatile int; // expected-error {{'volatile' is invalid in friend declarations}}
+ template friend const class X; // expected-error {{'const' is invalid in friend declarations}}
+ // C++ doesn't have restrict and _Atomic, but they're both the same sort
+ // of qualifier.
+ typedef int *PtrToInt;
+ friend __restrict PtrToInt; // expected-error {{'restrict' is invalid in friend declarations}} \
+ // expected-error {{restrict requires a pointer or reference}}
+ friend _Atomic int; // expected-error {{'_Atomic' is invalid in friend declarations}}
// OK
int friend foo(void);
+ const int friend foo2(void);
friend int;
- friend const volatile int;
friend
float;
Index: cfe/trunk/lib/Sema/SemaDeclCXX.cpp
===
--- cfe/trunk/lib/Sema/SemaDeclCXX.cpp
+++ cfe/trunk/lib/Sema/SemaDeclCXX.cpp
@@ -14017,6 +14017,29 @@
assert(DS.isFriendSpecified());
assert(DS.getStorageClassSpec() == DeclSpec::SCS_unspecified);
+ // C++ [class.friend]p3:
+ // A friend declaration that does not declare a function shall have one of
+ // the following forms:
+ // friend elaborated-type-specifier ;
+ // friend simple-type-specifier ;
+ // friend typename-specifier ;
+ //
+ // Any declaration with a type qualifier does not have that form. (It's
+ // legal to specify a qualified type as a friend, you just can't write the
+ // keywords.)
+ if (DS.getTypeQualifiers()) {
+if (DS.getTypeQualifiers() & DeclSpec::TQ_const)
+ Diag(DS.getConstSpecLoc(), diag::err_friend_decl_spec) << "const";
+if (DS.getTypeQualifiers() & DeclSpec::TQ_volatile)
+ Diag(DS.getVolatileSpecLoc(), diag::err_friend_decl_spec) << "volatile";
+if (DS.getTypeQualifiers() & DeclSpec::TQ_restrict)
+ Diag(DS.getRestrictSpecLoc(), diag::err_friend_decl_spec) << "restrict";
+if (DS.getTypeQualifiers() & DeclSpec::TQ_atomic)
+ Diag(DS.getAtomicSpecLoc(), diag::err_friend_decl_spec) << "_Atomic";
+if (DS.getTypeQualifiers() & DeclSpec::TQ_unaligned)
+ Diag(DS.getUnalignedSpecLoc(), diag::err_friend_decl_spec) << "__unaligned";
+ }
+
// Try to convert the decl specifier to a type. This works for
// friend templates because ActOnTag never produces a ClassTemplateDecl
//
[PATCH] D43105: [RISCV] Enable __int128_t and uint128_t through clang flag
efriedma added a comment.
> This flag can then be used to build compiler-rt for RISCV32.
Can you give a little more context for why this is necessary? As far as I
know, compiler-rt generally supports building for targets which don't have
__int128_t. (If all riscv targets are supposed to support __int128_t, you can
just turn it on without adding a flag.)
Comment at: lib/Basic/Targets/RISCV.h:85
+ bool hasInt128Type(const LangOptions &Opts) const override {
+return Opts.UseInt128;
+ }
Maybe make this a cross-platform flag, rather than riscv-specific?
Comment at: test/Driver/types.c:4
+// RUN: not %clang -c --target=riscv32-unknown-linux-gnu %s \
+// RUN: 2>&1 | FileCheck %s
+
This test won't work unless the riscv backend is enabled; maybe use
-fsyntax-only?
Repository:
rC Clang
https://reviews.llvm.org/D43105
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43105: [RISCV] Enable __int128_t and uint128_t through clang flag
efriedma added a comment. So you want int128_t for compiler-rt itself, so you can use the soft-float implementation, but you want to make int128_t opt-in to avoid the possibility of someone getting a link error trying to link code built with clang against libgcc.a? That seems a little convoluted, but I guess it's okay. Not sure I like the name "-fuse-int128"; doesn't really indicate what it's doing. Maybe "-fforce-enable-int128". Repository: rC Clang https://reviews.llvm.org/D43105 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43105: [RISCV] Enable __int128_t and uint128_t through clang flag
efriedma added inline comments. Comment at: include/clang/Driver/Options.td:843 +def fforce_enable_int128 : Flag<["-"], "fforce-enable-int128">, Group, + Flags<[CC1Option]>, HelpText<"Enable support for int128_t type">; We should have an inverse flag -fno-force-enable-int128, like we do for other "-f" flags. (Not sure anyone is actually likely to use it, but better to have it in case someone needs it.) Repository: rC Clang https://reviews.llvm.org/D43105 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43105: [RISCV] Enable __int128_t and uint128_t through clang flag
efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. LGTM Please post a follow-up to add this to the 7.0 release notes. Repository: rC Clang https://reviews.llvm.org/D43105 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43540: [WebAssembly] Enable -Werror=strict-prototypes by default
efriedma added a comment. If someone is compiling C code that doesn't have undefined behavior, it should work; if it doesn't, that's a clear bug. (As far as I know, there shouldn't be any issues here, but if there are, file a bug and CC me.) WebAssembly is not the only platform where varargs and non-varargs calls are incompatible. Some other popular platforms where this is a problem are 64-bit Windows and hard-float AArch32. So I don't think it makes sense to enable this specifically for one target. Repository: rL LLVM https://reviews.llvm.org/D43540 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43423: [SimplifyCFG] Create flag to disable simplifyCFG.
efriedma added a comment. > FWIW, I'd very much prefer the details of the optimizer wouldn't be exposed > as frontend flags. Yes, definitely this. Frontend flags to control the optimizer should state an expected result, not just turn off some completely arbitrary set of transforms. Otherwise, we're likely to end up in a situation in the future where we add a new optimization, or split an existing pass, and it isn't clear which transforms the frontend flag should apply to. https://reviews.llvm.org/D43423 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D31972: Do not force the frame pointer by default for ARM EABI
efriedma added reviewers: t.p.northover, efriedma.
efriedma added inline comments.
Comment at: lib/Driver/ToolChains/Clang.cpp:569
+ if (Triple.getEnvironment() == llvm::Triple::EABI) {
+switch (Triple.getArch()) {
Specifically checking for "llvm::Triple::EABI" is suspicious... what are you
trying to distinguish?
https://reviews.llvm.org/D31972
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33774: [CodeGen] Make __attribute__(const) calls speculatable
efriedma added a comment.
Consider something like this:
define i32 @div(i32 %x, i32 %y) {
entry:
%div = sdiv i32 %x, %y
ret i32 %div
}
We can mark this function readnone, but not speculatable: it doesn't read or
write memory, but could exhibit undefined behavior.
Consider another function:
@x = common local_unnamed_addr global i32 0, align 4
define i32 @get_x() {
entry:
%0 = load i32, i32* @x, align 4, !tbaa !2
ret i32 %0
}
We can mark this function speculatable, but not readnone: it doesn't exhibit
undefined behavior or have side-effects, but it does read memory.
Given that, it seems a little dubious to translate `__attribute__((const))`
into `speculatable`.
https://reviews.llvm.org/D33774
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33765: Show correct column nr. when multi-byte utf8 chars are used.
efriedma added a comment. Correctly counting columns is a bit more complicated that that... for example, consider what happens if you replace `ideëen` with `idez̈en`. See https://stackoverflow.com/questions/3634627/how-to-know-the-preferred-display-width-in-columns-of-unicode-characters . https://reviews.llvm.org/D33765 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D31972: Do not force the frame pointer by default for ARM EABI
efriedma added inline comments.
Comment at: lib/Driver/ToolChains/Clang.cpp:569
+ if (Triple.getEnvironment() == llvm::Triple::EABI) {
+switch (Triple.getArch()) {
chrib wrote:
> efriedma wrote:
> > Specifically checking for "llvm::Triple::EABI" is suspicious... what are
> > you trying to distinguish?
> I'm targeting the AAPCS for bare toolsets, (we could also test EABIHF by the
> way)
>
> I'm not sure about the other ABIs (such as llvm::Triple::OpenBSD) so it is
> probably conservative and stick to what I can test. Do you think this
> pertains to more, for instance to AAPCS-LINUX, without breaking anything ?
>
So... something like isTargetAEABI() in ARMSubtarget.h?
Please clarify the comment, and add a check for EABIHF.
https://reviews.llvm.org/D31972
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33926: [clang] Remove double semicolons. NFC.
efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. You don't need to ask for review for a trivial change like this. https://reviews.llvm.org/D33926 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D31972: Do not force the frame pointer by default for ARM EABI
efriedma added a comment. Please start a thread on cfe-dev about this; most developers don't read cfe-commits, and thinking about it a bit more, I'm not confident omitting frame pointers is really a good default. I would guess there's existing code which depends on frame pointers to come up with a stack trace, since table-based unwinding is complicated and takes up a lot of space. Grepping over the in-tree tests, it looks like the "eabi" triples used in practice are "arm-non-eabi", "arm-netbsd-eabi" and variants of "thumbv7m-apple-darwin-eabi" (and variants of those). Please make sure you aren't changing the behavior of netbsd and "Darwin" targets. https://reviews.llvm.org/D31972 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33910: [ubsan] Detect invalid unsigned pointer index expression (clang)
efriedma added inline comments. Comment at: cfe/trunk/lib/CodeGen/CGExprScalar.cpp:2666 + bool isSigned = indexOperand->getType()->isSignedIntegerOrEnumerationType(); + bool mayHaveNegativeGEPIndex = isSigned || isSubtraction; + This logic doesn't look quite right; what happens here if you write "p - SIZE_MAX"? Repository: rL LLVM https://reviews.llvm.org/D33910 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34121: [ubsan] Teach the pointer overflow check that "p - <= p" (PR33430)
efriedma added a comment.
Looks okay, but I haven't been reviewing this series of patches closely, so I
could be missing something.
Comment at: lib/CodeGen/CGExprScalar.cpp:3974
ValidGEP = Builder.CreateAnd(PosOrZeroValid, NoOffsetOverflow);
+ } else if (!SignedIndices && IsSubtraction) {
+auto *NegOrZeroValid = Builder.CreateICmpULE(ComputedGEP, IntPtr);
Please make this an "else" rather than an "else if" (you can assert the inside
the else body if it's helpful).
https://reviews.llvm.org/D34121
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34523: AST: mangle BlockDecls under MS ABI
efriedma added a comment. Patch is missing context. You have to use getBlockManglingNumber() for blocks which are externally visible; otherwise, the numbers won't be consistent in other translation units. Repository: rL LLVM https://reviews.llvm.org/D34523 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34523: AST: mangle BlockDecls under MS ABI
efriedma added a comment. > @efriedma hmm...using getBlockManglingNumber causes the name to be > duplicated. Ill look into that. Have you looked at the Itanium mangling implementation? > However, wouldn't all the block invocation functions be defined and COMDAT'ed? IIRC, we always emit blocks with internal linkage, not linkonce. But even if we did use linkonce linkage, the block could get inlined. Repository: rL LLVM https://reviews.llvm.org/D34523 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34568: [Sema] Make BreakContinueFinder handle nested loops.
efriedma created this revision.
We don't care about break or continue statements that aren't
associated with the current loop, so make sure the visitor
doesn't find them.
Fixes https://bugs.llvm.org/show_bug.cgi?id=32648 .
Repository:
rL LLVM
https://reviews.llvm.org/D34568
Files:
lib/Sema/SemaStmt.cpp
test/Sema/loop-control.c
test/SemaCXX/warn-loop-analysis.cpp
Index: test/SemaCXX/warn-loop-analysis.cpp
===
--- test/SemaCXX/warn-loop-analysis.cpp
+++ test/SemaCXX/warn-loop-analysis.cpp
@@ -202,6 +202,12 @@
if (true) continue;
i--;
}
+
+ // But do warn if the continue is in a nested loop.
+ for (;;i--) { // expected-note{{decremented here}}
+for (int j = 0; j < 10; ++j) continue;
+i--; // expected-warning{{decremented both}}
+ }
}
struct iterator {
@@ -259,6 +265,12 @@
if (true) continue;
i--;
}
+
+ // But do warn if the continue is in a nested loop.
+ for (;;i--) { // expected-note{{decremented here}}
+for (int j = 0; j < 10; ++j) continue;
+i--; // expected-warning{{decremented both}}
+ }
}
int f(int);
Index: test/Sema/loop-control.c
===
--- test/Sema/loop-control.c
+++ test/Sema/loop-control.c
@@ -119,3 +119,51 @@
for ( ; ({ ++y; break; y;}); ++y) {} // expected-warning{{'break' is bound to loop, GCC binds it to switch}}
}
}
+
+void pr32648_1(int x, int y) {
+ switch(x) {
+ case 1:
+for ( ; ({ ++y; switch (y) { case 0: break; } y;}); ++y) {} // no warning
+ }
+}
+
+void pr32648_2(int x, int y) {
+ while(x) {
+for ( ; ({ ++y; switch (y) { case 0: continue; } y;}); ++y) {} // expected-warning {{'continue' is bound to current loop, GCC binds it to the enclosing loop}}
+ }
+}
+
+void pr32648_3(int x, int y) {
+ switch(x) {
+ case 1:
+for ( ; ({ ++y; for (; y; y++) { break; } y;}); ++y) {} // no warning
+ }
+}
+
+void pr32648_4(int x, int y) {
+ switch(x) {
+ case 1:
+for ( ; ({ ++y; for (({ break; }); y; y++) { } y;}); ++y) {} // expected-warning{{'break' is bound to loop, GCC binds it to switch}}
+ }
+}
+
+void pr32648_5(int x, int y) {
+ switch(x) {
+ case 1:
+for ( ; ({ ++y; while (({ break; y; })) {} y;}); ++y) {} // expected-warning{{'break' is bound to current loop, GCC binds it to the enclosing loop}}
+ }
+}
+
+void pr32648_6(int x, int y) {
+ switch(x) {
+ case 1:
+for ( ; ({ ++y; do {} while (({ break; y; })); y;}); ++y) {} // expected-warning{{'break' is bound to current loop, GCC binds it to the enclosing loop}}
+ }
+}
+
+void pr32648_7(int x, int y) {
+ switch(x) {
+ case 1:
+for ( ; ({ ++y; do { break; } while (y); y;}); ++y) {} // no warning
+ }
+}
Index: lib/Sema/SemaStmt.cpp
===
--- lib/Sema/SemaStmt.cpp
+++ lib/Sema/SemaStmt.cpp
@@ -1544,23 +1544,78 @@
// A visitor to determine if a continue or break statement is a
// subexpression.
- class BreakContinueFinder : public EvaluatedExprVisitor {
+ class BreakContinueFinder : public ConstEvaluatedExprVisitor {
SourceLocation BreakLoc;
SourceLocation ContinueLoc;
+bool InSwitch = false;
+
public:
-BreakContinueFinder(Sema &S, Stmt* Body) :
+BreakContinueFinder(Sema &S, const Stmt* Body) :
Inherited(S.Context) {
Visit(Body);
}
-typedef EvaluatedExprVisitor Inherited;
+typedef ConstEvaluatedExprVisitor Inherited;
-void VisitContinueStmt(ContinueStmt* E) {
+void VisitContinueStmt(const ContinueStmt* E) {
ContinueLoc = E->getContinueLoc();
}
-void VisitBreakStmt(BreakStmt* E) {
- BreakLoc = E->getBreakLoc();
+void VisitBreakStmt(const BreakStmt* E) {
+ if (!InSwitch)
+BreakLoc = E->getBreakLoc();
+}
+
+void VisitSwitchStmt(const SwitchStmt* S) {
+ if (const Stmt *Init = S->getInit())
+Visit(Init);
+ if (const Stmt *CondVar = S->getConditionVariableDeclStmt())
+Visit(CondVar);
+ if (const Stmt *Cond = S->getCond())
+Visit(Cond);
+
+ // Don't return break statements from the body of a switch.
+ InSwitch = true;
+ if (const Stmt *Body = S->getBody())
+Visit(Body);
+ InSwitch = false;
+}
+
+void VisitForStmt(const ForStmt *S) {
+ // Only visit the init statement of a for loop; the body
+ // has a different break/continue scope.
+ if (const Stmt *Init = S->getInit())
+Visit(Init);
+}
+
+void VisitWhileStmt(const WhileStmt *) {
+ // Do nothing; the children of a while loop have a different
+ // break/continue scope.
+}
+
+void VisitDoStmt(const DoStmt *) {
+ // Do nothing; the children of a while loop have a different
+ // break/continue scope.
+}
+
+void VisitCXXForRangeStmt(const CXXForRangeStmt *S) {
+ // Only visit the initialization of a for loop; the body
+ // has
[PATCH] D34523: AST: mangle BlockDecls under MS ABI
efriedma added a comment. I think we just use "0" as a sentinel value to indicate the block doesn't need a mangling number. getLambdaManglingNumber works the same way? See CXXNameMangler::mangleUnqualifiedBlock in the Itanium mangler. Repository: rL LLVM https://reviews.llvm.org/D34523 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34523: AST: mangle BlockDecls under MS ABI
efriedma added a comment. > How do we end up in a state where the block invocation function is > referenced external to the TU? ODR allows certain definitions, like class definitions and inline function definitions, to be written in multiple translation units. See http://itanium-cxx-abi.github.io/cxx-abi/abi.html#closure-types , specifically the list of cases where lambda types are required to correspond. Repository: rL LLVM https://reviews.llvm.org/D34523 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D41149: [CodeGen] Specialize mixed-sign mul-with-overflow (fix PR34920)
efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. LGTM Comment at: lib/CodeGen/CGBuiltin.cpp:912 + auto IntMax = + llvm::APInt::getMaxValue(ResultInfo.Width).zextOrSelf(Op1Info.Width); + llvm::Value *TruncOverflow = CGF.Builder.CreateICmpUGT( zext() rather than zextOrSelf(). https://reviews.llvm.org/D41149 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D41374: [Coverage] Fix use-after free in coverage emission
efriedma created this revision.
efriedma added reviewers: vsk, davidxl.
efriedma added a project: clang.
Fixes regression from r320533.
This fixes the undefined behavior, but I'm not sure it's really right... I
think we end up with missing coverage for code in modules.
Repository:
rC Clang
https://reviews.llvm.org/D41374
Files:
lib/CodeGen/CodeGenModule.cpp
Index: lib/CodeGen/CodeGenModule.cpp
===
--- lib/CodeGen/CodeGenModule.cpp
+++ lib/CodeGen/CodeGenModule.cpp
@@ -4289,7 +4289,11 @@
}
void CodeGenModule::EmitDeferredUnusedCoverageMappings() {
- for (const auto &Entry : DeferredEmptyCoverageMappingDecls) {
+ // We call takeVector() here to avoid use-after-free.
+ // FIXME: DeferredEmptyCoverageMappingDecls is getting mutated because
+ // we deserialize function bodies to emit coverage info for them, and that
+ // deserializes more declarations. How should we handle that case?
+ for (const auto &Entry : DeferredEmptyCoverageMappingDecls.takeVector()) {
if (!Entry.second)
continue;
const Decl *D = Entry.first;
Index: lib/CodeGen/CodeGenModule.cpp
===
--- lib/CodeGen/CodeGenModule.cpp
+++ lib/CodeGen/CodeGenModule.cpp
@@ -4289,7 +4289,11 @@
}
void CodeGenModule::EmitDeferredUnusedCoverageMappings() {
- for (const auto &Entry : DeferredEmptyCoverageMappingDecls) {
+ // We call takeVector() here to avoid use-after-free.
+ // FIXME: DeferredEmptyCoverageMappingDecls is getting mutated because
+ // we deserialize function bodies to emit coverage info for them, and that
+ // deserializes more declarations. How should we handle that case?
+ for (const auto &Entry : DeferredEmptyCoverageMappingDecls.takeVector()) {
if (!Entry.second)
continue;
const Decl *D = Entry.first;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D41374: [Coverage] Fix use-after free in coverage emission
This revision was automatically updated to reflect the committed changes.
Closed by commit rL321052: [Coverage] Fix use-after free in coverage emission
(authored by efriedma, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D41374?vs=127440&id=127451#toc
Repository:
rL LLVM
https://reviews.llvm.org/D41374
Files:
cfe/trunk/lib/CodeGen/CodeGenModule.cpp
Index: cfe/trunk/lib/CodeGen/CodeGenModule.cpp
===
--- cfe/trunk/lib/CodeGen/CodeGenModule.cpp
+++ cfe/trunk/lib/CodeGen/CodeGenModule.cpp
@@ -4289,7 +4289,11 @@
}
void CodeGenModule::EmitDeferredUnusedCoverageMappings() {
- for (const auto &Entry : DeferredEmptyCoverageMappingDecls) {
+ // We call takeVector() here to avoid use-after-free.
+ // FIXME: DeferredEmptyCoverageMappingDecls is getting mutated because
+ // we deserialize function bodies to emit coverage info for them, and that
+ // deserializes more declarations. How should we handle that case?
+ for (const auto &Entry : DeferredEmptyCoverageMappingDecls.takeVector()) {
if (!Entry.second)
continue;
const Decl *D = Entry.first;
Index: cfe/trunk/lib/CodeGen/CodeGenModule.cpp
===
--- cfe/trunk/lib/CodeGen/CodeGenModule.cpp
+++ cfe/trunk/lib/CodeGen/CodeGenModule.cpp
@@ -4289,7 +4289,11 @@
}
void CodeGenModule::EmitDeferredUnusedCoverageMappings() {
- for (const auto &Entry : DeferredEmptyCoverageMappingDecls) {
+ // We call takeVector() here to avoid use-after-free.
+ // FIXME: DeferredEmptyCoverageMappingDecls is getting mutated because
+ // we deserialize function bodies to emit coverage info for them, and that
+ // deserializes more declarations. How should we handle that case?
+ for (const auto &Entry : DeferredEmptyCoverageMappingDecls.takeVector()) {
if (!Entry.second)
continue;
const Decl *D = Entry.first;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D41421: Eliminate a magic number. NFC.
efriedma added a comment. Please use llvm::array_lengthof to compute the length of an array. Alternatively, there's an overload of PP.getSpelling which takes a SmallVector and returns a StringRef as a parameter; you could change this code to use it, which would remove the need for the check. https://reviews.llvm.org/D41421 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D41421: Eliminate a magic number. NFC.
efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. LGTM https://reviews.llvm.org/D41421 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40698: [ubsan] Diagnose noreturn functions which return
efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. LGTM https://reviews.llvm.org/D40698 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33563: Track whether a unary operation can overflow
efriedma added inline comments.
Comment at: include/clang/AST/Expr.h:1728
+ UnaryOperator(Expr *input, Opcode opc, QualType type, ExprValueKind VK,
+ExprObjectKind OK, SourceLocation l, bool CanOverflow = false)
+ : Expr(UnaryOperatorClass, type, VK, OK,
Is the default argument necessary here? Better to avoid when possible.
Comment at: lib/Sema/SemaExpr.cpp:12212
case UO_Minus:
+CanOverflow = isOverflowingIntegerType(Context, Input.get()->getType());
Input = UsualUnaryConversions(Input.get());
UO_Plus can't overflow.
Comment at: test/Misc/ast-dump-stmt.c:66
+ // CHECK:ImplicitCastExpr
+ // CHECK: DeclRefExpr{{.*}}'T2' 'int'
+}
What does it mean for bitwise complement to "overflow"?
https://reviews.llvm.org/D33563
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D23934: Add a -ffixed-date-time= flag that sets the initial value of __DATE__, __TIME__, __TIMESTAMP__
efriedma added inline comments. Comment at: lib/Frontend/CompilerInvocation.cpp:2252 + TM.tm_isdst = -1; + mktime(&TM); + Opts.FixedDateTime = TM; Does using mktime like this depend on the local timezone? https://reviews.llvm.org/D23934 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D41717: [CGBuiltin] Handle unsigned mul overflow properly (PR35750)
efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. LGTM https://reviews.llvm.org/D41717 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D41780: Preserve unknown STDC pragma through preprocessor
efriedma added a comment.
If you move all the #pragma STDC handlers from the lexer to the parser, you
might be able to avoid adding an explicit STDC handler in
PrintPreprocessedOutput.cpp.
Comment at: test/Preprocessor/pragma_unknown.c:32
#pragma STDC SO_GREAT // expected-warning {{unknown pragma in STDC namespace}}
#pragma STDC // expected-warning {{unknown pragma in STDC namespace}}
Maybe add CHECK lines to make sure these come out correctly?
Repository:
rC Clang
https://reviews.llvm.org/D41780
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D41780: Preserve unknown STDC pragma through preprocessor
efriedma added a comment. Should be safe, I think; currently, FENV_ACCESS and CX_LIMITED_RANGE have no effect, and when we do start supporting them, we'll probably want to handle them in the parser, like we do for FP_CONTRACT. Repository: rC Clang https://reviews.llvm.org/D41780 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D41780: Preserve unknown STDC pragma through preprocessor
efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. LGTM Repository: rC Clang https://reviews.llvm.org/D41780 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33563: Track whether a unary operation can overflow
efriedma added inline comments.
Comment at: test/Misc/ast-dump-stmt.c:66
+ // CHECK:ImplicitCastExpr
+ // CHECK: DeclRefExpr{{.*}}'T2' 'int'
+}
aaron.ballman wrote:
> efriedma wrote:
> > What does it mean for bitwise complement to "overflow"?
> When the sign bit flips on a signed value, e.g., `~0`.
Bitwise complement always flips the sign bit; calling that an "overflow"
doesn't seem useful.
https://reviews.llvm.org/D33563
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33563: Track whether a unary operation can overflow
efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. LGTM https://reviews.llvm.org/D33563 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D41517: mmintrin.h documentation fixes and updates
efriedma added inline comments. Comment at: lib/Headers/mmintrin.h:55 /// -/// This intrinsic corresponds to the VMOVD / MOVD instruction. +/// This intrinsic corresponds to the MOVD instruction. /// craig.topper wrote: > kromanova wrote: > > I tried clang on Linux, x86_64, and if -mavx option is passed, we generate > > VMOVD, if this option is omitted, we generate MOVD. > > I think I understand the rational behind this change (namely, to keep MOVD, > > but remove VMOVD), > > since this intrinsic should use MMX registers and shouldn't have > > corresponding AVX instruction(s). > > > > However, that's what we generate at the moment when -mavx is passed (I > > suspect because our MMX support is limited) > > vmovd %edi, %xmm0 > > > > Since we are writing the documentation for clang compiler, we should > > document what clang compiler is doing, not what is should be doing. > > Craig, what do you think? Should we revert back to VMOVD/MOVD? > > > We can change it back to VMOVD/MOVD The reference to vmovd seems confusing. Yes, LLVM compiles `_mm_movpi64_epi64(_mm_cvtsi32_si64(i))` to vmovd, but that doesn't mean either of those intrinsics "corresponds" to vmovd; that's just the optimizer combining two operations into one. https://reviews.llvm.org/D41517 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D41516: emmintrin.h documentation fixes and updates
efriedma added inline comments.
Comment at: cfe/trunk/lib/Headers/emmintrin.h:4683
///
-/// This intrinsic has no corresponding instruction.
+/// This intrinsic corresponds to the MOVDQ2Q instruction.
///
kromanova wrote:
> kromanova wrote:
> > I'm not sure about this change.
> >
> > Intel documentation says they generate MOVDQ2Q (don't have icc handy to
> > try).
> > However, I've tried on Linux/X86_64 with clang and gcc, - and we just
> > return.
> >
> Though I suspect it's possible to generate movdq2q, I couldn't come up with
> an test to trigger this instruction generation.
> Should we revert this change?
>
>
> ```
> __m64 fooepi64_pi64 (__m128i a, __m128 c)
> {
> __m64 x;
>
> x = _mm_movepi64_pi64 (a);
> return x;
> }
>
> ```
>
> on Linux we generate return instruction.
> I would expect (v)movq %xmm0,%rax to be generated instead of retq.
> Am I missing something? Why do we return 64 bit integer in xmm register
> rather than in %rax?
>
The x86-64 calling convention rules say that __m64 is passed/returned in SSE
registers.
Try the following, which generates movdq2q:
```
__m64 foo(__m128i a, __m128 c)
{
return _mm_add_pi8(_mm_movepi64_pi64(a), _mm_set1_pi8(5));
}
```
Repository:
rL LLVM
https://reviews.llvm.org/D41516
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D41516: emmintrin.h documentation fixes and updates
efriedma added inline comments.
Comment at: cfe/trunk/lib/Headers/emmintrin.h:4683
///
-/// This intrinsic has no corresponding instruction.
+/// This intrinsic corresponds to the MOVDQ2Q instruction.
///
kromanova wrote:
> efriedma wrote:
> > kromanova wrote:
> > > kromanova wrote:
> > > > I'm not sure about this change.
> > > >
> > > > Intel documentation says they generate MOVDQ2Q (don't have icc handy to
> > > > try).
> > > > However, I've tried on Linux/X86_64 with clang and gcc, - and we just
> > > > return.
> > > >
> > > Though I suspect it's possible to generate movdq2q, I couldn't come up
> > > with an test to trigger this instruction generation.
> > > Should we revert this change?
> > >
> > >
> > > ```
> > > __m64 fooepi64_pi64 (__m128i a, __m128 c)
> > > {
> > > __m64 x;
> > >
> > > x = _mm_movepi64_pi64 (a);
> > > return x;
> > > }
> > >
> > > ```
> > >
> > > on Linux we generate return instruction.
> > > I would expect (v)movq %xmm0,%rax to be generated instead of retq.
> > > Am I missing something? Why do we return 64 bit integer in xmm register
> > > rather than in %rax?
> > >
> > The x86-64 calling convention rules say that __m64 is passed/returned in
> > SSE registers.
> >
> > Try the following, which generates movdq2q:
> > ```
> > __m64 foo(__m128i a, __m128 c)
> > {
> > return _mm_add_pi8(_mm_movepi64_pi64(a), _mm_set1_pi8(5));
> > }
> > ```
> Thanks! That explains it :)
> I can see that MOVDQ2Q gets generated.
>
> What about intrinsic below, _mm_movpi64_epi64? Can we ever generate
> MOVD+VMOVQ as stated in the review?
> Or should we write VMOVQ / MOVQ?
Testcase:
```
#include
__m128 foo(__m128i a, __m128 c)
{
return _mm_movpi64_epi64(_mm_add_pi8(_mm_movepi64_pi64(a), _mm_set1_pi8(5)));
}
```
In this case, we should generate movq2dq, but currently don't (I assume due to
a missing DAGCombine). I don't see how you could ever get MOVD... but see
https://reviews.llvm.org/rL321898, which could be the source of some confusion.
Repository:
rL LLVM
https://reviews.llvm.org/D41516
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40673: Add _Float128 as alias to __float128 to enable compilations on Fedora27/glibc2-26
efriedma added a comment. > as this patch is committed clang is broken (cannot use glibc headers) This patch was supposed to *fix* compatibility with the glibc headers. If it doesn't, we should clearly revert it... but we need to figure out what we need to do to be compatible first. From what I can see on this thread, we *must* define _Float128 on x86-64 Linux, and we *must not* define _Float128 for any other glibc target. Is that correct? Or does it depend on the glibc version? (We have a bit of time before the 6.0 release, so we can adjust the behavior here to make it work. We probably don't want to try to add full _Float128 support on the branch, though.) Repository: rC Clang https://reviews.llvm.org/D40673 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43734: [RecordLayout] Don't align to non-power-of-2 sizes when using -mms-bitfields
efriedma added inline comments. Comment at: cfe/trunk/lib/AST/RecordLayoutBuilder.cpp:1758 + Context.getTargetInfo().getTriple().isWindowsGNUEnvironment()) && +"Non PowerOf2 size outside of GNU mode"); +if (TypeSize > FieldAlign && This assertion seems weird. `sizeof(long double)` is 12 for other targets, including x86-32 Linux. Repository: rL LLVM https://reviews.llvm.org/D43734 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43734: [RecordLayout] Don't align to non-power-of-2 sizes when using -mms-bitfields
efriedma added inline comments. Comment at: cfe/trunk/lib/AST/RecordLayoutBuilder.cpp:1758 + Context.getTargetInfo().getTriple().isWindowsGNUEnvironment()) && +"Non PowerOf2 size outside of GNU mode"); +if (TypeSize > FieldAlign && mstorsjo wrote: > efriedma wrote: > > This assertion seems weird. `sizeof(long double)` is 12 for other targets, > > including x86-32 Linux. > Perhaps it'd make more sense to flip it around, `assert(isPowerOf2() || > !Triple.isWindowsMSVCEnvironment())`? Yes, that makes sense. Repository: rL LLVM https://reviews.llvm.org/D43734 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43908: [RecordLayout] Only assert that fundamental type sizes are power of two on MSVC
efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. LGTM Repository: rC Clang https://reviews.llvm.org/D43908 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44223: [ms] Emit vtordisp initializers in a deterministic order.
efriedma added inline comments.
Comment at: lib/CodeGen/MicrosoftCXXABI.cpp:1202
+ SmallVector VBases(VBaseMap.begin(), VBaseMap.end());
+ std::stable_sort(VBases.begin(), VBases.end(),
+ [](const VBaseEntry &a, const VBaseEntry &b) {
Using stable_sort() here, as opposed to sort(), doesn't do anything useful
here; VBaseMap is a DenseMap with a pointer key, so the input is in random
order anyway.
https://reviews.llvm.org/D44223
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44582: [Builtins] Fix calling long double math functions on x86_64 mingw
efriedma added a comment. Can we just fix the bug in the backend, rather than trying to hack around it in clang? Repository: rC Clang https://reviews.llvm.org/D44582 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44582: [Builtins] Fix calling long double math functions on x86_64 mingw
efriedma added a comment. The backend has code to generate SRet returns, which is used when TargetLowering::CanLowerReturn returns false. Probably a tiny change to X86CallingConv.td to use that codepath on Windows. Repository: rC Clang https://reviews.llvm.org/D44582 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44646: Sema: in msvc compatibility mode, don't allow forceinline on variadics
efriedma added a comment. The compiler shouldn't inline functions which call va_start, whether or not they're marked always_inline. That should work correctly, I think, at least on trunk. (See https://reviews.llvm.org/D42556 .) If you want to warn anyway, that's okay. Repository: rC Clang https://reviews.llvm.org/D44646 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46013: [ARM] Conform to AAPCS when passing overaligned composites as arguments
efriedma added reviewers: aemerson, rjmccall. efriedma added a comment. I'm not sure Apple will want to mess with their ABI like this... adding some reviewers. Otherwise LGTM. Comment at: lib/CodeGen/TargetInfo.cpp:5790 // than ABI alignment. - uint64_t ABIAlign = 4; - uint64_t TyAlign = getContext().getTypeAlign(Ty) / 8; - if (getABIKind() == ARMABIInfo::AAPCS_VFP || - getABIKind() == ARMABIInfo::AAPCS) -ABIAlign = std::min(std::max(TyAlign, (uint64_t)4), (uint64_t)8); - + uint64_t ABIAlign = 32; + uint64_t TyAlign; I'd rather do alignment computations in bytes, rather than bits (can we use getTypeAlignInChars here?) https://reviews.llvm.org/D46013 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46112: Allow _Atomic to be specified on incomplete types
efriedma added a comment.
If you don't want to spend too much time on C++, fine; could you add a short
Objective-C test instead to make sure the trivially-copyable checks are working?
What are the changes to Sema::RequireCompleteTypeImpl supposed to do?
Comment at: test/Sema/atomic-type.c:30
+ int i;
+ (void)__atomic_load(addr, &i, 5); // expected-error {{address argument to
atomic operation must be a pointer to a trivially-copyable type ('_Atomic(void)
*' invalid)}}
+ (void)__c11_atomic_load(addr, 5); // expected-error {{invalid use of
incomplete type 'void'}}
This error message is terrible; yes, technically 'void' isn't trivially
copyable, but that isn't really helpful.
https://reviews.llvm.org/D46112
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47628: Detect an incompatible VLA pointer assignment
efriedma added inline comments.
Comment at: lib/AST/ASTContext.cpp:8588
+ Expr *E = VAT->getSizeExpr();
+ if (E && VAT->getSizeExpr()->isIntegerConstantExpr(TheInt, *this))
+return std::make_pair(true, TheInt);
`E && E->isIntegerConstantExpr`?
Comment at: lib/AST/ASTContext.cpp:8603
+ std::tie(HaveRSize, RSize) = SizeFetch(RVAT, RCAT);
+ if (HaveLSize && HaveRSize && LSize != RSize)
+return {}; // Definite, but unequal, array dimension
The != will hit an assertion failure if LSize and RSize don't have the same
bitwidth.
Repository:
rC Clang
https://reviews.llvm.org/D47628
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46013: [ARM] Conform to AAPCS when passing overaligned composites as arguments
efriedma added a comment. > I'm not sure it's appropriate to impose this as an overhead on all targets. You mean the overhead of increasing the size of TypeInfo? That's fair. https://reviews.llvm.org/D46013 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47628: Detect an incompatible VLA pointer assignment
efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. LGTM https://reviews.llvm.org/D47628 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
