[PATCH] D59557: Fix CodeGen/arm64-microsoft-status-reg.cpp test

efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59557/new/ https://reviews.llvm.org/D59557 ___

[PATCH] D28213: [Frontend] Correct values of ATOMIC_*_LOCK_FREE to match builtin

efriedma added a comment. It looks like it was reverted because it was breaking i386 BSD, where __GCC_ATOMIC_LLONG_LOCK_FREE is in fact supposed to be "1" (because cmpxchg8b isn't always available). Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D28213/new/ https:

[PATCH] D28213: [Frontend] Correct values of ATOMIC_*_LOCK_FREE to match builtin

efriedma added a comment. > Do we care about cases where it *might* be available? i.e. can we say it's > never available instead? That doesn't really help here... the fundamental issue is that getMaxAtomicInlineWidth() is wrong (or at least, was wrong at the time this was initially merged; I h

[PATCH] D59557: Fix CodeGen/arm64-microsoft-status-reg.cpp test

efriedma added a comment. Usually no... I wasn't think about it the last time I reviewed a change to this file. Patch welcome to just zap it, assuming we have appropriate coverage in llvm/test/CodeGen/AArch64. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59557

[PATCH] D28213: [Frontend] Correct values of ATOMIC_*_LOCK_FREE to match builtin

efriedma added a comment. It's kind of awkward to use ">=" on a CPU enum, but yes, that's the right idea. Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D28213/new/ https://reviews.llvm.org/D28213 ___ cfe-commits mailin

[PATCH] D59655: [AArch64] Split the neon.addp intrinsic into integer and fp variants

efriedma added inline comments. Comment at: llvm/lib/IR/AutoUpgrade.cpp:574 + if (ArgTy->getElementType()->isFloatingPointTy()) { +auto fArgs = F->getFunctionType()->params(); +Type *Tys[] = {fArgs[0], fArgs[1]}; This code is weird... you're

[PATCH] D59566: [X86] Correct the value of MaxAtomicInlineWidth for pre-586 cpus

efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. LGTM > This is needed to get the LLONG_LOCK_FREE macro to be 2 on i586 and greater. Not sure these tests are really valuable, given we plan to change that immediately after this is merged

[PATCH] D59655: [AArch64] Split the neon.addp intrinsic into integer and fp variants

efriedma added a comment. The IR at this Comment at: llvm/lib/IR/AutoUpgrade.cpp:574 + if (ArgTy->getElementType()->isFloatingPointTy()) { +auto fArgs = F->getFunctionType()->params(); +Type *Tys[] = {fArgs[0], fArgs[1]}; aemerson wrote: >

[PATCH] D59655: [AArch64] Split the neon.addp intrinsic into integer and fp variants

efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59655/new/ https://reviews.llvm.org/D59655 ___

[PATCH] D58236: Make address space conversions a bit stricter.

efriedma added a comment. If nobody else agrees with my position on this, I'm not going to continue arguing on the explicit cast behavior. But please add a testcase showing that at least `(global int**)(void*)(local int**)p` works without an error. CHANGES SINCE LAST ACTION https://reviews.

[PATCH] D59776: [Sema] Don't check for array bounds when the types in the base expression are dependent.

efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. LGTM Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59776/new/ https://reviews.llvm.org/D59776 ___ cfe-commit

[PATCH] D58797: [Sema] Add some compile time _FORTIFY_SOURCE diagnostics

efriedma added a comment. For that particular case, I think we could suppress the false positive using DiagRuntimeBehavior. How many total false positives are we talking about, and how many can the compiler statically prove are unreachable? Repository: rC Clang CHANGES SINCE LAST ACTION

[PATCH] D60349: [COFF, ARM64] Fix ABI implementation of struct returns

efriedma added inline comments. Comment at: lib/CodeGen/MicrosoftCXXABI.cpp:1062 + bool sretInX0 = (CGM.getTarget().getTriple().getArch() == + llvm::Triple::aarch64) && !RD->isPOD(); + ostannard wrote: > This should also check aarch64_be. aarch

[PATCH] D60674: [X86] Restore the pavg intrinsics.

efriedma added a comment. > Though I modified the avx512 intrinsics to not have masking built in. Do we need autoupgrade support from the old avx512 intrinsics to the new avx512 intrinsics? Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60674/new/ https://reviews

[PATCH] D60674: [X86] Restore the pavg intrinsics.

efriedma added a comment. Okay, thanks. Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60674/new/ https://reviews.llvm.org/D60674 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/m

[PATCH] D60349: [COFF, ARM64] Fix ABI implementation of struct returns

efriedma added inline comments. Comment at: lib/Sema/SemaDeclCXX.cpp:5906 +bool isAArch64 = S.Context.getTargetInfo().getTriple().isAArch64(); +if (isAArch64 && !D->isAggregate()) + return false; The isAggregate predicate is not appropriate to check

[PATCH] D60349: [COFF, ARM64] Fix ABI implementation of struct returns

efriedma added inline comments. Comment at: lib/Sema/SemaDeclCXX.cpp:5904 if (CCK == TargetInfo::CCK_MicrosoftWin64) { +bool isAArch64 = S.Context.getTargetInfo().getTriple().isAArch64(); I'm not entirely sure it makes sense to do all of these Windows-sp

[PATCH] D60349: [COFF, ARM64] Fix ABI implementation of struct returns

efriedma added a comment. It looks like there's some missing documentation in the ARM64 ABI document involving homogeneous aggregates... in particular, it looks like non-POD types are never homogeneous, or something along those lines. I guess we can address that in a followup, though. @TomTan

[PATCH] D60349: [COFF, ARM64] Fix ABI implementation of struct returns

efriedma added a comment. > For NotPod, it is aggregate which is specific in the document Yes, it's an aggregate which is returned in registers... but it's returned in integer registers, unlike Pod which is returned in floating-point registers. CHANGES SINCE LAST ACTION https://reviews.llvm.

[PATCH] D60349: [COFF, ARM64] Fix ABI implementation of struct returns

efriedma added inline comments. Comment at: lib/CodeGen/MicrosoftCXXABI.cpp:1074 + if (!RD->hasTrivialCopyAssignment()) +return true; + return false; richard.townsend.arm wrote: > richard.townsend.arm wrote: > > Should this function also check for user-prov

[PATCH] D60349: [COFF, ARM64] Fix ABI implementation of struct returns

efriedma added a comment. > Return info for HFA and HVA is updated That's helpful, but not really sufficient; according to the AAPCS rules, both "Pod" and "NotPod" from my testcase are HFAs. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60349/new/ https://reviews.llvm.org/D60349 __

[PATCH] D60349: [COFF, ARM64] Fix ABI implementation of struct returns

efriedma added a comment. > Could you provide some more details on why NotPod is HFA, according to AAPCS? In general, computing whether a composite type is a "homogeneous aggregate" according to section 4.3.5 depends only on the "fundamental data types". It looks through "aggregates" (C struct

[PATCH] D60349: [COFF, ARM64] Fix ABI implementation of struct returns

efriedma added inline comments. Comment at: lib/CodeGen/MicrosoftCXXABI.cpp:1078 +return true; + if (RD->hasUserDeclaredConstructor()) +return true; Like I mentioned before, the rule says "user-provided", not "user-declared". Comment a

[PATCH] D60349: [COFF, ARM64] Fix ABI implementation of struct returns

efriedma added a comment. I'd like to see a few more tests to cover the interesting cases that came up during development of this patch: 1. The user-provided constructor issue: there should be testcases with an explicit user-provided constructor, a non-trivial constructor that's explicitly def

[PATCH] D61225: [COFF, ARM64] Align global symbol by size for ARM64 MSVC ABI

efriedma added subscribers: mgrang, rsmith. efriedma added a comment. I was going to ask if local variables are also supposed to be aligned this way... but I guess there's no way for standard C++ code to tell without explicitly making weird alignment assumptions, so let's not worry about that.

[PATCH] D60349: [COFF, ARM64] Fix ABI implementation of struct returns

efriedma added inline comments. Comment at: lib/CodeGen/MicrosoftCXXABI.cpp:1106 + +FI.getReturnInfo().setInReg(isAArch64 && !IsSizeGreaterThan128(RD)); richard.townsend.arm wrote: > I'm not sure what the IsSizeGreaterThan128 check is doing here - if the >

[PATCH] D60349: [COFF, ARM64] Fix ABI implementation of struct returns

efriedma added inline comments. Comment at: lib/CodeGen/MicrosoftCXXABI.cpp:1106 + +FI.getReturnInfo().setInReg(isAArch64 && !IsSizeGreaterThan128(RD)); richard.townsend.arm wrote: > efriedma wrote: > > richard.townsend.arm wrote: > > > I'm not sure what th

[PATCH] D61392: [clang] Handle lround/llround builtins

efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. Looks fine, once the LLVM changes are settled. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61392/new/ https://reviews.llvm.org/D61392

[PATCH] D61225: [COFF, ARM64] Align global symbol by size for ARM64 MSVC ABI

efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. LGTM Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61225/new/ https://reviews.llvm.org/D61225 ___ cfe-commit

[PATCH] D60349: [COFF, ARM64] Fix ABI implementation of struct returns

efriedma accepted this revision. efriedma added a comment. LGTM, assuming it passes testing on electron CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60349/new/ https://reviews.llvm.org/D60349 ___ cfe-commits mailing list cfe-commits@lists.

[PATCH] D61634: [clang/llvm] Allow efficient implementation of libc's memory functions in C/C++

efriedma added a comment. I would be careful about trying to over-generalize here. There are a few different related bits of functionality which seem to be interesting, given the discussion in the llvm-dev thread, here, and in related patches: 1. The ability to specify -fno-builtin* on a per-f

[PATCH] D50229: [ARM][AArch64] Add feature +fp16fml

efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. LGTM https://reviews.llvm.org/D50229 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/c

[PATCH] D30806: [nonnull] Teach Clang to attach the nonnull LLVM attribute to declarations and calls instead of just definitions, and then teach it to *not* attach such attributes even if the source c

efriedma closed this revision. efriedma added a comment. This was merged in https://reviews.llvm.org/rC298491 . https://reviews.llvm.org/D30806 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cf

[PATCH] D52695: [clang][Parse] Diagnose useless null statements (PR39111)

efriedma added a comment. Again, I'm not sure we really want this... we don't really like adding new off-by-default warnings, and we don't usually add diagnostics to enforce style. Comment at: include/clang/Basic/DiagnosticGroups.td:163 def CXX11ExtraSemi : DiagGroup<"c++11-e

[PATCH] D52811: [COFF, ARM64] Add _InterlockedAdd intrinsic

efriedma added inline comments. Comment at: test/CodeGen/ms-intrinsics.c:379 +// CHECK: [[RESULT:%[0-9]+]] = atomicrmw add i32* %value, i32 %mask seq_cst +// CHECK: ret i32 [[RESULT:%[0-9]+]] +// CHECK: } Missing "add" instruction. _InterlockedAdd is suppose

[PATCH] D52807: [COFF, ARM64] Add _InterlockedCompareExchangePointer_nf intrinsic

efriedma added inline comments. Comment at: lib/CodeGen/CGBuiltin.cpp:3003 + case Builtin::BI_InterlockedCompareExchangePointer: + case Builtin::BI_InterlockedCompareExchangePointer_nf: { llvm::Type *RTy; mgrang wrote: > rnk wrote: > > Is the no fence vers

[PATCH] D46535: Correct warning on Float->Integer conversions.

efriedma added a comment. > Every Integer is representable (lossy of course) as a float as far as I know. Casting a __uint128_t to float can overflow. And overflowing a _Float16 is easy. (Of course, both __uint128_t and _Float16 are rare in normal C/C++ code.) Repository: rC Clang https:/

[PATCH] D52811: [COFF, ARM64] Add _InterlockedAdd intrinsic

efriedma added a comment. > How come this hasn't been an issue for those targets up until now? MSVC doesn't define _InterlockedAdd for x64. Repository: rC Clang https://reviews.llvm.org/D52811 ___ cfe-commits mailing list cfe-commits@lists.llvm.

[PATCH] D52838: [COFF, ARM64] Add __getReg intrinsic

efriedma added a comment. read_register only allows reading reserved registers because reading an allocatable register is meaningless (the compiler can store arbitrary data in allocatable registers). The same applies to __getReg; given that, I would assume real code doesn't use anything other

[PATCH] D50179: [AArch64][ARM] Context sensitive meaning of option "crypto"

efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. LGTM https://reviews.llvm.org/D50179 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/c

[PATCH] D52838: [COFF, ARM64] Add __getReg intrinsic

efriedma added inline comments. Comment at: lib/CodeGen/CGBuiltin.cpp:6589 +std::string Reg = StrVal == "31" ? "sp" : + "x" + std::string(StrVal.c_str()); + Could you just write this as `std::string Reg = Value == 31 ? "sp" : "x" + Value

[PATCH] D52838: [COFF, ARM64] Add __getReg intrinsic

efriedma added a comment. Needs a testcase for the error message like we have for other intrinsics in test/Sema/builtins-arm64.c . Otherwise LGTM. https://reviews.llvm.org/D52838 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists

[PATCH] D52838: [COFF, ARM64] Add __getReg intrinsic

efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. LGTM https://reviews.llvm.org/D52838 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/c

[PATCH] D52807: [COFF, ARM64] Add _InterlockedCompareExchangePointer_nf intrinsic

efriedma added a comment. How are you testing? You might see different behavior at -O0. Repository: rC Clang https://reviews.llvm.org/D52807 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/c

[PATCH] D52807: [COFF, ARM64] Add _InterlockedCompareExchangePointer_nf intrinsic

efriedma added inline comments. Comment at: lib/CodeGen/CGBuiltin.cpp:3003 + case Builtin::BI_InterlockedCompareExchangePointer: + case Builtin::BI_InterlockedCompareExchangePointer_nf: { llvm::Type *RTy; efriedma wrote: > rnk wrote: > > mgrang wrote: > >

[PATCH] D52807: [COFF, ARM64] Add _InterlockedCompareExchangePointer_nf intrinsic

efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. LGTM https://reviews.llvm.org/D52807 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/c

[PATCH] D52807: [COFF, ARM64] Add _InterlockedCompareExchangePointer_nf intrinsic

efriedma added inline comments. Comment at: lib/CodeGen/CGBuiltin.cpp:3003 + case Builtin::BI_InterlockedCompareExchangePointer: + case Builtin::BI_InterlockedCompareExchangePointer_nf: { llvm::Type *RTy; efriedma wrote: > efriedma wrote: > > rnk wrote: >

[PATCH] D52811: [COFF, ARM64] Add _InterlockedAdd intrinsic

efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. LGTM https://reviews.llvm.org/D52811 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/c

[PATCH] D52854: Use is.constant intrinsic for __builtin_constant_p

efriedma added a comment. I don't think that was the approach @rsmith was suggesting; you don't need to wrap every possible kind of expression. Rather, at the point where the expression is required to be constant, add a single expression which wraps the entire expression tree to indicate that.

[PATCH] D56571: [RFC prototype] Implementation of asm-goto support in clang

efriedma added inline comments. Comment at: lib/Analysis/CFG.cpp:3137 + + Block = createBlock(false); + Block->setTerminator(G); Passing add_successor=false seems suspect; this could probably use more test coverage. Comment at: lib/CodeGen/C

[PATCH] D56571: [RFC prototype] Implementation of asm-goto support in clang

efriedma added a comment. Forgot about protected scopes... This patch is missing code and testcases for JumpScopeChecker. (The behavior should be roughly equivalent to what we do for indirect goto.) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56571/new/ https://reviews.llvm.org/D56

[PATCH] D57631: [COFF, ARM64] Add ARM64 support for MS intrinsic _fastfail

efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. LGTM Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57631/new/ https://reviews.llvm.org/D57631 ___ cfe-commit

[PATCH] D57636: [COFF, ARM64] Fix types for _ReadStatusReg, _WriteStatusReg

efriedma added a subscriber: TomTan. efriedma added a comment. Missing testcase changes. (It should be possible to check that we aren't inserting incorrect truncation/extension operations in the IR.) Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57636/new/ http

[PATCH] D57497: [RISCV] Passing small data limitation value to RISCV backend

efriedma added a comment. How do we actually want this to work for LTO? Would it make sense to encode this on global variables somehow, to allow different thresholds for different source files? Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57497/new/ https://r

[PATCH] D57636: [COFF, ARM64] Fix types for _ReadStatusReg, _WriteStatusReg

efriedma added a comment. Yes, we should fix CodeGenFunction::EmitAArch64BuiltinExpr to eliminated the unnecessary calls to CreateZext/CreateTrunc. (With this patch, they're no-ops, but better to clean up the code.) Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/

[PATCH] D56571: [RFC prototype] Implementation of asm-goto support in clang

efriedma added inline comments. Comment at: lib/Sema/JumpDiagnostics.cpp:347 + LabelAndGotoScopes[S] = ParentScope; + Jumps.push_back(S); +} This doesn't look right; I think we need to add it to IndirectJumps instead. This probably impacts a testc

[PATCH] D56571: [RFC prototype] Implementation of asm-goto support in clang

efriedma added inline comments. Comment at: lib/Sema/JumpDiagnostics.cpp:347 + LabelAndGotoScopes[S] = ParentScope; + Jumps.push_back(S); +} jyu2 wrote: > efriedma wrote: > > This doesn't look right; I think we need to add it to IndirectJumps > > i

[PATCH] D57636: [COFF, ARM64] Fix types for _ReadStatusReg, _WriteStatusReg

efriedma added a comment. In test/CodeGen/arm64-microsoft-status-reg.cpp, you can write something like `// CHECK-IR: %[[VAR:.*]] = call i64 @llvm.read_register.i64(metadata ![[MD2:.*]])`, then `// CHECK-IR-NEXT: store i64 %[[VAR]]` on the next line. See also http://llvm.org/docs/CommandGuide/Fi

[PATCH] D57915: [COFF, ARM64] Remove definitions for _byteswap library functions

efriedma added a comment. Should clang IR generation be lowering these to bswap calls anyway? Even if the function technically exists in the ucrt, it's going to be pretty slow to call it. Please leave the tests; fix the CHECK lines, if necessary, but we should still check it compiles. Repos

[PATCH] D56571: [RFC prototype] Implementation of asm-goto support in clang

efriedma added inline comments. Comment at: lib/AST/Stmt.cpp:628 + DiagOffs = CurPtr-StrStart-1; + return diag::err_asm_invalid_operand_for_goto_labels; +} jyu2 wrote: > rsmith wrote: > > jyu2 wrote: > > > rsmith wrote: > > > > I'm curio

[PATCH] D57915: [COFF, ARM64] Remove definitions for _byteswap library functions

efriedma added a comment. I did some quick testing with MSVC; apparently it inlines the implementations of these functions when optimizations are on. We definitely want to support inlining these. Since these are commonly used in performance-sensitive code, I'd prefer to implement the required

[PATCH] D57915: [COFF, ARM64] Remove definitions for _byteswap library functions

efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. I guess we can track inlining separately, if you want to merge this quickly to unblock the Chrome build. LGTM > the former provides global declaration which seems inherited by the > defi

[PATCH] D57636: [COFF, ARM64] Fix types for _ReadStatusReg, _WriteStatusReg

efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. LGTM. Do you want me to commit this for you? Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57636/new/ https://reviews.llvm.org/D57636 __

[PATCH] D57636: [COFF, ARM64] Fix types for _ReadStatusReg, _WriteStatusReg

This revision was automatically updated to reflect the committed changes. Closed by commit rL353493: [COFF, ARM64] Fix types for _ReadStatusReg, _WriteStatusReg (authored by efriedma, committed by ). Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Changed prior to commit:

[PATCH] D57935: [Sema] Make string literal init an rvalue.

efriedma created this revision. efriedma added a reviewer: rsmith. Herald added a project: clang. This allows substantially simplifying the expression evaluation code, because we don't have to special-case lvalues which are actually string literal initialization. This currently throws away an o

[PATCH] D57935: [Sema] Make string literal init an rvalue.

efriedma closed this revision. efriedma added a comment. https://reviews.llvm.org/rC353569 Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57935/new/ https://reviews.llvm.org/D57935 ___ cfe-commits mailing list cfe-com

[PATCH] D58069: [Sema] Mark GNU compound literal array init as an rvalue.

efriedma created this revision. efriedma added a reviewer: rsmith. Herald added a project: clang. Basically the same issue as string init, except it didn't really have any visible consequences before I removed the implicit lvalue-to-rvalue conversion from CodeGen. While I'm here, a couple minor

[PATCH] D58069: [Sema] Mark GNU compound literal array init as an rvalue.

efriedma marked 2 inline comments as done. efriedma added inline comments. Comment at: lib/Sema/SemaInit.cpp:172-173 + E = PE->getSubExpr(); +else if (UnaryOperator *UO = dyn_cast(E)) + E = UO->getSubExpr(); +else if (GenericSelectionExpr *GSE = dyn_cast(E)) ---

[PATCH] D58069: [Sema] Mark GNU compound literal array init as an rvalue.

This revision was automatically updated to reflect the committed changes. efriedma marked an inline comment as done. Closed by commit rC353762: [Sema] Mark GNU compound literal array init as an rvalue. (authored by efriedma, committed by ). Changed prior to commit: https://reviews.llvm.org/D580

[PATCH] D57497: [RISCV] Passing small data limitation value to RISCV backend

efriedma added a comment. > Did you mean declare as a target feature in RISCV.td or I misunderstanding > something? That's sort of the right idea, but I don't think it works in this context because we aren't trying to change the generated code for a function; we actually need to stick the glob

[PATCH] D56305: [AArch64] Support reserving arbitrary general purpose registers

efriedma added inline comments. Comment at: clang/lib/Driver/ToolChains/Arch/AArch64.cpp:318 + if (Args.hasArg(options::OPT_ffixed_x0)) +Features.push_back("+reserve-x0"); trong wrote: > trong wrote: > > phosek wrote: > > > trong wrote: > > > > What happen

[PATCH] D58120: [Builtins] Treat `bcmp` as a builtin.

efriedma added a comment. This looks essentially fine, but I'd like to see some basic test coverage for the changes to warnings and constant evaluation. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58120/new/ https://reviews.llvm.org/D58120 _

[PATCH] D58153: [Driver] Default all Android ARM targets to NEON.

efriedma added a comment. The official documentation still says "Your app must perform runtime detection to confirm that NEON-capable machine code can be run on the target device" (https://developer.android.com/ndk/guides/cpu-arm-neon#runtime_detection). Is that wrong? Repository: rG LLVM

[PATCH] D58120: [Builtins] Treat `bcmp` as a builtin.

efriedma accepted this revision. efriedma added a comment. LGTM Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58120/new/ https://reviews.llvm.org/D58120 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https:/

[PATCH] D58149: [clang] Make sure C99/C11 features in are provided in C++11

efriedma added a comment. Formally, I don't think C11 is a normative reference for C++11 or C++14, only C++17 (see [intro.refs] in the standard). Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58149/new/ https://reviews.llvm.org/D58149

[PATCH] D56571: [RFC prototype] Implementation of asm-goto support in clang

efriedma added a comment. I think this is in good shape. Comment at: lib/Sema/JumpDiagnostics.cpp:675 + // asm-goto. + //if (!IsAsmGoto && IndirectJumpTargets.empty()) { + if (JumpTargets.empty()) { Commented-out code? We probably don't need a diagnostic fo

[PATCH] D58289: [clang] Only provide C11 features in starting with C++17

efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. LGTM Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58289/new/ https://reviews.llvm.org/D58289 ___ cfe-commit

[PATCH] D58375: [Clang][NewPM] Disable tests that are broken under new PM

efriedma added a comment. I can understand why tests that use -O1 or -O2 would produce different results with the new pass manager, but it looks like not all the tests are like that. Do you know why those tests are failing? For the tests that do use -O, instead of marking them unsupported, cou

[PATCH] D56990: Bugfix for Replacement of tied operand of inline asm

efriedma added inline comments. Comment at: test/Sema/inline-asm-x86-constraint.c:2 +// REQUIRES: x86-registered-target +// RUN: %clang_cc1 %s -o %t +typedef unsigned char __attribute__((vector_size(8))) _m64u8; Test belongs in test/CodeGen. Please use -emit-ll

[PATCH] D57335: [IR] Don't assume all functions are 4 byte aligned

efriedma added a subscriber: hfinkel. efriedma added a comment. Chandler, when you have a chance, can you look at the LangRef changes, since you put some thought into the design? I think the DataLayout/LangRef changes look correct. I agree it isn't necessary to fix every target in the initial p

[PATCH] D37042: Teach clang to tolerate the 'p = nullptr + n' idiom used by glibc

efriedma added a comment. It would be nice to warn on any nullptr arithmetic, yes. (We probably want the wording of the warning to be a bit different if we're activating this workaround.) https://reviews.llvm.org/D37042 ___ cfe-commits mailing li

[PATCH] D37042: Teach clang to tolerate the 'p = nullptr + n' idiom used by glibc

efriedma added a reviewer: rsmith. efriedma added a comment. I didn't think of this earlier, but strictly speaking, I think "(char*)nullptr+0" isn't undefined in C++? But probably worth emitting the warning anyway; anyone writing out arithmetic on null is probably doing something suspicious, e

[PATCH] D37042: Teach clang to tolerate the 'p = nullptr + n' idiom used by glibc

efriedma added inline comments. Comment at: lib/Sema/SemaExpr.cpp:8808 +Context.getTypeSize(pointerType) == +Context.getTypeSize(IExp->getType())) + IsGNUIdiom = true; rsmith wrote: > andrew.w.kaylor wrote: > > efriedma wrote: > > > Plea

[PATCH] D37042: Teach clang to tolerate the 'p = nullptr + n' idiom used by glibc

efriedma added inline comments. Comment at: lib/Sema/SemaExpr.cpp:8808 +Context.getTypeSize(pointerType) == +Context.getTypeSize(IExp->getType())) + IsGNUIdiom = true; andrew.w.kaylor wrote: > efriedma wrote: > > rsmith wrote: > > > andr

[PATCH] D36487: Emit section information for extern variables.

efriedma added inline comments. Comment at: lib/CodeGen/CodeGenModule.cpp:2434 +// Emit section information for extern variables. +if (D->hasExternalStorage() && !D->isThisDeclarationADefinition()) { + if (const SectionAttr *SA = D->getAttr()) eandre

[PATCH] D37448: Fix cast assertion on MS inline assembly with vector spills (PR34021)

efriedma added a comment. > According to our definition, v4si is NOT a "Vector" type, since a vector type > requires it be a FP value. Umm, what? An integer vector is still a vector. The backend will return it in xmm0 on x86 (both 32 and 64-bit). The actual problem here is that X86_32Target

[PATCH] D31140: [LLVMbugs] [Bug 18710] Only generate .ARM.exidx and .ARM.extab when needed in EHABI

efriedma added a comment. If you want to follow what we do for x86-64 on ARM, you should be doing this in the driver, not codegen (grep for IsUnwindTablesDefault). https://reviews.llvm.org/D31140 ___ cfe-commits mailing list cfe-commits@lists.llvm.

[PATCH] D31140: [LLVMbugs] [Bug 18710] Only generate .ARM.exidx and .ARM.extab when needed in EHABI

efriedma added a comment. Oh, you don't want to emit them by default. :) I'm not sure what you're trying to do here... there are three possibilities: 1. The function could have an exception thrown through it, so we need an unwind table. 2. The function can't have an exception thrown through it,

[PATCH] D35235: [libc++] Replace __sync_* functions with __libcpp_atomic_* functions

efriedma added a comment. Ping. https://reviews.llvm.org/D35235 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D37042: Teach clang to tolerate the 'p = nullptr + n' idiom used by glibc

efriedma added inline comments. Comment at: include/clang/Basic/DiagnosticSemaKinds.td:6031 InGroup; +def ext_gnu_null_ptr_arith : Extension< + "arithmetic on a null pointer treated as a cast from integer to pointer is a GNU extension">, "extension" isn't re

[PATCH] D37042: Teach clang to tolerate the 'p = nullptr + n' idiom used by glibc

efriedma added inline comments. Comment at: include/clang/Basic/DiagnosticSemaKinds.td:6031 InGroup; +def ext_gnu_null_ptr_arith : Extension< + "arithmetic on a null pointer treated as a cast from integer to pointer is a GNU extension">, andrew.w.kaylor wrot

[PATCH] D37042: Teach clang to tolerate the 'p = nullptr + n' idiom used by glibc

efriedma added inline comments. Comment at: include/clang/Basic/DiagnosticSemaKinds.td:6031 InGroup; +def ext_gnu_null_ptr_arith : Extension< + "arithmetic on a null pointer treated as a cast from integer to pointer is a GNU extension">, andrew.w.kaylor wrot

[PATCH] D31140: [LLVMbugs] [Bug 18710] Only generate .ARM.exidx and .ARM.extab when needed in EHABI

efriedma added inline comments. Comment at: lib/Driver/ToolChains/BareMetal.cpp:61 +bool BareMetal::IsUnwindTablesDefault(const ArgList &Args) const { + return getDriver().CCCIsCXX(); +} This still seems weird. In most situations, I would expect you want the sa

[PATCH] D37861: preserving #pragma clang assume_nonnull in preprocessed output

efriedma added inline comments. Comment at: lib/Frontend/PrintPreprocessedOutput.cpp:566 + MoveToLine(Loc); + OS << "#pragma " << "clang assume_nonnull end"; + setEmittedDirectiveOnThisLine(); Extra "<<"? Comment at: test/Preprocessor/pragma

[PATCH] D37042: Teach clang to tolerate the 'p = nullptr + n' idiom used by glibc

efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. LGTM https://reviews.llvm.org/D37042 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/c

[PATCH] D37861: preserving #pragma clang assume_nonnull in preprocessed output

efriedma added a comment. It looks like you uploaded a diff against the previous version of the patch instead of trunk? https://reviews.llvm.org/D37861 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/li

[PATCH] D37042: Teach clang to tolerate the 'p = nullptr + n' idiom used by glibc

efriedma added a comment. I agree, it doesn't add much value. Either way, though, please make sure you address the buildbot failures quickly. Repository: rL LLVM https://reviews.llvm.org/D37042 ___ cfe-commits mailing list cfe-commits@lists.llvm

[PATCH] D31140: [LLVMbugs] [Bug 18710] Only generate .ARM.exidx and .ARM.extab when needed in EHABI

efriedma added a comment. I would rather not make ARM baremetal do something different from every other target... On Linux, for most targets, we don't add the uwtable attribute by default; without the uwtable attribute, non-ARM backends (e.g. aarch64) only emit tables for functions which might

[PATCH] D38060: Remove offset size check in nullptr arithmetic handling

efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. LGTM https://reviews.llvm.org/D38060 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/c

[PATCH] D38046: [Atomic][X8664] set max atomic inline/promote width according to the target

efriedma added a comment. Needs testcase. Comment at: lib/Basic/Targets/X86.h:898 + MaxAtomicPromoteWidth = 64; + MaxAtomicInlineWidth = 64; +} I don't think we need to mess with MaxAtomicPromoteWidth? Probably more intuitive to check "if (hasFea

<    1   2   3   4   5   6   7   8   9   10   >