[PATCH] D87615: [clang][Driver] Force stack realignment on 32-bit Solaris/x86

efriedma added a comment. In D87615#2271297 , @ro wrote: > In D87615#2271268 , @RKSimon wrote: > >> Would it be possible to add some tests? > > Probably not reliably: the tests that usually fail bye the hundreds hap

[PATCH] D87607: [clang][aarch64] Support implicit casts between GNU and SVE vectors

efriedma added inline comments. Comment at: clang/lib/AST/ASTContext.cpp:8522 + return VT->getElementType().getCanonicalType() == + getBuiltinVectorTypeInfo(BT).ElementType; } We allow casting SVE fixed-width vectors only if the el

[PATCH] D87607: [clang][aarch64] Support implicit casts between GNU and SVE vectors

efriedma added inline comments. Comment at: clang/lib/AST/ASTContext.cpp:8522 + return VT->getElementType().getCanonicalType() == + getBuiltinVectorTypeInfo(BT).ElementType; } c-rhodes wrote: > efriedma wrote: > > We allow casting

[PATCH] D87615: [X86] Fix stack alignment on 32-bit Solaris/x86

efriedma added inline comments. Comment at: llvm/lib/Target/X86/X86Subtarget.cpp:270 + else if (isTargetSolaris() && !In64BitMode) +stackAlignment = Align(4); stackAlignment is initialized to 4 in the header, so `stackAlignment = Align(4)` here is a no-op

[PATCH] D87615: [X86] Fix stack alignment on 32-bit Solaris/x86

efriedma added a comment. Also, it would be nice to have some regression test coverage; add a Solaris RUN line to llvm/test/CodeGen/X86/stack-align2.ll ? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87615/new/ https://reviews.llvm.org/D87615 ___

[PATCH] D87615: [X86] Fix stack alignment on 32-bit Solaris/x86

efriedma accepted this revision. efriedma added a comment. LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87615/new/ https://reviews.llvm.org/D87615 ___ cfe-commits mailing list cfe-commits@lists.llv

[PATCH] D87607: [clang][aarch64] Support implicit casts between GNU and SVE vectors

efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. LGTM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87607/new/ https://reviews.llvm.org/D87607 ___ cfe-commits mailing list cfe-commits

[PATCH] D85118: [clang][AArch64] Correct return type of Neon vqmovun intrinsics

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/D85118/new/ https://reviews.llvm.org/D85118 _

[PATCH] D88009: [AArch64] Fix return type of Neon scalar comparison intrinsics

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/D88009/new/ https://reviews.llvm.org/D88009 _

[PATCH] D86790: [FE] Use preferred alignment instead of ABI alignment for complete object when applicable

efriedma added inline comments. Comment at: clang/lib/CodeGen/TargetInfo.cpp:4539 CharUnits CCAlign = getParamTypeAlignment(Ty); CharUnits TyAlign = getContext().getTypeAlignInChars(Ty); jyknight wrote: > Xiangling_L wrote: > > jasonliu wrote: > > > Q

[PATCH] D86101: [AArch64][SVE] Add missing debug info for ACLE types.

efriedma added inline comments. Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:734 + if (Info.ElementType == CGM.getContext().BoolTy) +NumElemsPerVG /= 8; + I guess NumElemsPerVG of a bool vector is always divisible by 8 because we don't expose `` et

[PATCH] D86065: [SVE] Make ElementCount members private

efriedma added a comment. > Perhaps now would be a good time to combine TypeSize and ElementCount into a > single Polynomial type? We don't have to implement the whole abstraction of > c*x^n (since we currently don't use the exponent, and don't distinguish > between X's) but if it's ever needed

[PATCH] D86187: [X86] Add support 'tune' in target attribute

efriedma added inline comments. Comment at: clang/test/Sema/attr-target.c:26 +//expected-warning@+1 {{unsupported tune 'hiss' in the 'target' attribute string; 'target' attribute ignored}} +int __attribute__((target("tune=hiss,tune=woof"))) apple_tree() { return 4; } + -

[PATCH] D85191: [AST] Get field size in chars rather than bits in RecordLayoutBuilder.

efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. LGTM Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1847 +IsIncompleteArrayType ? CharUnits::Zero() : TI.first; +AlignIsRequired = Context.getTypeInfo(D->g

[PATCH] D86187: [X86] Add support 'tune' in target attribute

efriedma accepted this revision. efriedma added a comment. LGTM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86187/new/ https://reviews.llvm.org/D86187 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/ma

[PATCH] D86310: [X86] Align i128 to 16 bytes in x86-64 datalayout

efriedma added a comment. I'm afraid the AutoUpgrade component of this isn't compatible with existing IR without some additional work. I'm most concerned about cases like the following: #pragma pack(8) struct X { __int128 x; }; // Not a packed struct in IR because the native alignment is

[PATCH] D86310: [X86] Align i128 to 16 bytes in x86-64 datalayout

efriedma added a comment. As far as I know, there are basically three categories of things that depend on the alignment of a type. 1. The default alignment of load/store/alloca. On trunk, load/store/alloca always have explicitly specified alignment in memory. That said, old bitcode doesn't h

[PATCH] D86101: [AArch64][SVE] Add missing debug info for ACLE types.

efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. LGTM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86101/new/ https://reviews.llvm.org/D86101 ___ cfe-commits mailing list cfe-commits

[PATCH] D68720: Support -fstack-clash-protection for x86

efriedma closed this revision. efriedma added a comment. It was reverted, then re-landed. It's in trunk now. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D68720/new/ https://reviews.llvm.org/D68720 ___

[PATCH] D86621: [clang][Sparc] Default to -mcpu=v9 for SparcV8 on Solaris

efriedma added a comment. > While the fix proper is trivial: just two lines in > lib/Driver/ToolChains/CommonArgs.cpp, finding the right place has been > nightmarishly difficult: I'd have expected handling of a Solaris/SPARC CPU > default in either of Solaris or SPARC specific files, but not de

[PATCH] D86621: [clang][Sparc] Default to -mcpu=v9 for SparcV8 on Solaris

efriedma added inline comments. Comment at: clang/lib/Basic/Targets/Sparc.cpp:224 +Builder.defineMacro("__GCC_HAVE_SYNC_COMPARE_AND_SWAP_8"); + } } ro wrote: > efriedma wrote: > > This probably should be refactored so the target-independent code generates

[PATCH] D86621: [clang][Sparc] Default to -mcpu=v9 for SparcV8 on Solaris

efriedma added inline comments. Comment at: clang/lib/Basic/Targets/Sparc.cpp:224 +Builder.defineMacro("__GCC_HAVE_SYNC_COMPARE_AND_SWAP_8"); + } } ro wrote: > efriedma wrote: > > ro wrote: > > > efriedma wrote: > > > > This probably should be refactored so

[PATCH] D86621: [clang][Sparc] Default to -mcpu=v9 for SparcV8 on Solaris

efriedma added inline comments. Comment at: clang/lib/Driver/ToolChains/CommonArgs.cpp:350 return A->getValue(); +if (T.getArch() == llvm::Triple::sparc && T.isOSSolaris()) + return "v9"; ro wrote: > brad wrote: > > ro wrote: > > > efriedma wrote:

[PATCH] D86621: [clang][Sparc] Default to -mcpu=v9 for SparcV8 on Solaris

efriedma added inline comments. Comment at: clang/lib/Basic/Targets/Sparc.cpp:224 +Builder.defineMacro("__GCC_HAVE_SYNC_COMPARE_AND_SWAP_8"); + } } ro wrote: > efriedma wrote: > > ro wrote: > > > efriedma wrote: > > > > ro wrote: > > > > > efriedma wrote: >

[PATCH] D86621: [clang][Sparc] Default to -mcpu=v9 for SparcV8 on Solaris

efriedma added inline comments. Comment at: clang/lib/Basic/Targets/Sparc.cpp:224 +Builder.defineMacro("__GCC_HAVE_SYNC_COMPARE_AND_SWAP_8"); + } } ro wrote: > efriedma wrote: > > ro wrote: > > > efriedma wrote: > > > > ro wrote: > > > > > efriedma wrote: >

[PATCH] D87218: [builtins] Inline __paritysi2 into __paritydi2 and inline __paritydi2 into __parityti2.

efriedma accepted this revision. efriedma added a comment. LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87218/new/ https://reviews.llvm.org/D87218 ___ cfe-commits mailing list cfe-commits@lists.llv

[PATCH] D87358: [clang][aarch64] Fix ILP32 ABI for arm_sve_vector_bits

efriedma added inline comments. Comment at: clang/lib/AST/Type.cpp:2339 case BuiltinType::SveInt32: -return Ctx.IntTy; +return IsILP32 ? Ctx.LongTy : Ctx.IntTy; case BuiltinType::SveUint32: sdesmalen wrote: > Rather than comparing with a specific tr

[PATCH] D87358: [clang][aarch64] Fix ILP32 ABI for arm_sve_vector_bits

efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. LGTM Comment at: clang/lib/AST/Type.cpp:2324 // scalable and fixed-length vectors. -return Ctx.UnsignedCharTy; - case BuiltinType::SveInt16: -return Ctx.Sho

[PATCH] D92886: [Sema] Warn about unused functions even when they're inline

efriedma added a comment. I'm not sure this direction really makes sense. There's a long history of defining utility functions in headers as "static inline". Non-static inline functions in C have confusing semantics. In C++, the semantics are less confusing, but "static inline" still isn't ra

[PATCH] D90329: [PowerPC] Fix va_arg in C++, Objective-C on 32-bit ELF targets

efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. LGTM with one minor comment. Do you have commit access? If not, I can merge for you; please specify your preferred git "Author" line. Comment at: clang/test/CodeGenCXX

[PATCH] D77056: [Sema] Allow non-member operators for sizeless SVE types

efriedma added a reviewer: rsmith. efriedma added a subscriber: rsmith. efriedma added a comment. @rsmith, can you look at the changes to overloading? I haven't looked at that code in a very long time. Otherwise looks fine. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION htt

[PATCH] D87962: [clang] Change the multi-character character constants from extension to implementation-defined.

efriedma added a comment. > Or should we change the four character case to "Remark" so it wouldn't be > warned even with the "-pandemic" option? There seems no other choice. There's a DefaultIgnore modifier for warnings that turns then off by default. We use this sparingly, since it's hard to

[PATCH] D88789: [InstCombine] Revert rL226781 "Teach InstCombine to canonicalize loads which are only ever stored to always use a legal integer type if one is available." (PR47592)

efriedma added a comment. In D88789#2310606 , @chandlerc wrote: > FWIW, I still very much feel that this is the correct canonicalization, and > that downstream problems *must* be fixed downstream. Avoiding this > canonicalization doesn't actually fix the

[PATCH] D88013: [AArch64] Correct parameter type for unsigned Neon scalar shift intrinsics

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/D88013/new/ https://reviews.llvm.org/D88013 _

[PATCH] D88789: [InstCombine] Revert rL226781 "Teach InstCombine to canonicalize loads which are only ever stored to always use a legal integer type if one is available." (PR47592)

efriedma added a comment. > Keeping loads and stores of pointers as pointers to the extent possible > doesn't seem like a bad idea, but I'm worried people will feel like this > gives a *semantic* guarantee that isn't really there. Fundamentally, LLVM > still doesn't currently have typed memory.

[PATCH] D86447: [AST] Change return type of getTypeInfoInChars to a proper struct instead of std::pair.

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

[PATCH] D89130: [WIP][Sparc] Fix long double on 32-bit Solaris/SPARC

efriedma added a comment. SparcTargetLowering::LowerCall_32 is Sparc-specific code. Maybe you can take a look at where it's crashing, and ask some more specific question? The compiler-rt requirement for __uint128_t is a bit inconvenient, I guess; maybe it could be changed, but that's probably

[PATCH] D89362: [MS] Apply `inreg` to AArch64 sret parms on instance methods

efriedma added inline comments. Comment at: clang/lib/CodeGen/MicrosoftCXXABI.cpp:1077 -static bool hasMicrosoftABIRestrictions(const CXXRecordDecl *RD) { +static bool isCXX14Aggregate(const CXXRecordDecl *RD) { // For AArch64, we use the C++14 definition of an aggregate, so

[PATCH] D89362: [MS] Apply `inreg` to AArch64 sret parms on instance methods

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

[PATCH] D89523: PR44406: Follow behavior of array bound constant folding in more recent versions of GCC.

efriedma added a comment. Also update the documentation? See https://clang.llvm.org/docs/UsersManual.html#differences-between-various-standard-modes . Comment at: clang/lib/Sema/SemaDecl.cpp:5939 +ElemTy = TryToFixInvalidVariablyModifiedType(ElemTy, Context, +

[PATCH] D89523: PR44406: Follow behavior of array bound constant folding in more recent versions of GCC.

efriedma added inline comments. Comment at: clang/lib/Sema/SemaType.cpp:2278 ArraySize, &SizeVal, Diagnoser, - (S.LangOpts.GNUMode || S.LangOpts.OpenCL) ? Sema::AllowFold -: Sema::NoFold); + S.LangOpts.OpenCL ? Sema

[PATCH] D89523: PR44406: Follow behavior of array bound constant folding in more recent versions of GCC.

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/D89523/new/ https://reviews.llvm.org/D89523 _

[PATCH] D89573: [Driver] Incorporate -mfloat-abi in the computed triple on ARM

efriedma added inline comments. Comment at: clang/lib/Driver/ToolChain.cpp:807 +default: + break; +} Do we want to error in the "default" case? Not that anyone is likely to use -mfloat-abi on those targets, but I'd prefer not to silently miscompile

[PATCH] D89573: [Driver] Incorporate -mfloat-abi in the computed triple on ARM

efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. LGTM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89573/new/ https://reviews.llvm.org/D89573 ___ cfe-commits mailing list cfe-commits

[PATCH] D90329: [PowerPC] Fix va_arg in Objective-C on 32-bit ELF targets

efriedma added inline comments. Comment at: clang/lib/CodeGen/TargetInfo.cpp:4723 + bool isInt = Ty->isIntegerType() || Ty->hasPointerRepresentation() || + Ty->isAggregateType(); bool isF64 = Ty->isFloatingType() && getContext().getTypeSize(Ty) == 64; --

[PATCH] D99439: Update @llvm.powi to handle different int sizes for the exponent

efriedma added a comment. Seems reasonable. I'd like to see a test for autoupgrade; not sure if you need to make any code changes for that. Comment at: llvm/docs/LangRef.rst:13282 +to one of the ``__powi*`` functions in compiler-rt. Not all targets support all types however.

[PATCH] D98798: Produce warning for performing pointer arithmetic on a null pointer.

efriedma added a comment. @thakis if this is an issue for you, please revert; like you said, we can figure out the issues later. Looking at the warning you showed, we might want to consider suppressing that specific pattern, in addition to (or instead of?) suppressing it in system headers. The

[PATCH] D100388: [BROKEN][clang] Try to fix thunk function types

efriedma added a comment. > Would it be reasonable to instead start with a stopgap measure of not adding > attributes for this/return of thunks? You mean, add align attributes to "this" on regular methods, but not thunks? I can't see how that could do any harm. Comment at:

[PATCH] D100388: [BROKEN][clang] Try to fix thunk function types

efriedma added inline comments. Comment at: clang/lib/CodeGen/CGVTables.cpp:467 + if (!CalleeMD->isDefined()) { +CGM.ErrorUnsupported(ThunkGD.getDecl(), "thunk for forward declaration"); +return; lebedev.ri wrote: > efriedma wrote: > > "IsUnprototyped" m

[PATCH] D99439: Update @llvm.powi to handle different int sizes for the exponent

efriedma added inline comments. Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp:581 RTLIB::POWI_F128, RTLIB::POWI_PPCF128); if (!TLI.getLibcallName(LC)) { bjope wrote: > e

[PATCH] D100701: [clang][AVR] Redefine some types to be compatible with avr-gcc

efriedma added inline comments. Comment at: clang/lib/Basic/Targets/AVR.cpp:313 + Builder.defineMacro("__UINT16_TYPE__", "unsigned int"); + Builder.defineMacro("__INT16_TYPE__", "int"); Redefining `__INT16_TYPE__` like this is unusual. The macro is normally

[PATCH] D100701: [clang][AVR] Redefine some types to be compatible with avr-gcc

efriedma added inline comments. Comment at: clang/lib/Basic/Targets/AVR.cpp:313 + Builder.defineMacro("__UINT16_TYPE__", "unsigned int"); + Builder.defineMacro("__INT16_TYPE__", "int"); benshi001 wrote: > efriedma wrote: > > Redefining `__INT16_TYPE__` like t

[PATCH] D102015: [clang CodeGen] Don't crash on large atomic function parameter.

efriedma updated this revision to Diff 345543. efriedma added a comment. Figured out a way to test the EmitDelegateCallArg codepath. (A thunk doesn't work because the code is guarded by `!CurFuncIsThunk`.) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.

[PATCH] D102547: [clang][AVR] Redefine [u]int16_t to be compatible with avr-gcc

efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. LGTM, thanks. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D102547/new/ https://reviews.llvm.org/D102547 ___ cfe-commits mailing list

[PATCH] D99439: Update @llvm.powi to handle different int sizes for the exponent

efriedma added inline comments. Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp:581 RTLIB::POWI_F128, RTLIB::POWI_PPCF128); if (!TLI.getLibcallName(LC)) { bjope wrote: > e

[PATCH] D102015: [clang CodeGen] Don't crash on large atomic function parameter.

This revision was automatically updated to reflect the committed changes. Closed by commit rG698568b74c93: [clang CodeGen] Don't crash on large atomic function parameter. (authored by efriedma). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D102015/ne

[PATCH] D103082: [AArch64][SVE] Optimize svbool dupq ACLE intrinsic to fixed predicate patterns

efriedma added a comment. Do we really need a dedicated LLVM intrinsic to make the pattern-matching work here? It would be better if we could leverage @llvm.experimental.vector.insert.nxv16i8.v16i8 or something like that. Something along the lines of https://godbolt.org/z/Wz4azzKrP seems str

[PATCH] D103080: [CMake] Ignore arm_*.h for non-ARM build

efriedma added a comment. I think I'm fine with the general direction, but it would be nice to have a CMake option to force the headers for all targets to be built if someone needs them. For static analysis or something like that, I can imagine someone building LLVM without any backends at all

[PATCH] D103028: [clang][ARM] Remove arm2/3/6/7m CPU names

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/D103028/new/ https://reviews.llvm.org/D103028 ___

[PATCH] D103082: [AArch64][SVE] Improve codegen for dupq SVE ACLE intrinsics

efriedma added a comment. Can we add a few end-to-end tests of bool svdupq with constant operands to acle_sve_dupq.c? The pattern matching to create ptrue seems a bit fragile, so I want to make sure we don't break it by accident. Comment at: llvm/lib/Target/AArch64/AArch64Ta

[PATCH] D103611: Correct the behavior of va_arg checking in C++

efriedma added a comment. I'm a little nervous about using C type merging in C++; it's not designed to support C++ types, so it might end up crashing or something like that. I think I'd prefer to explicitly convert enum types to their underlying type. Repository: rG LLVM Github Monorepo CH

[PATCH] D103082: [AArch64][SVE] Improve codegen for dupq SVE ACLE intrinsics

efriedma added inline comments. Comment at: clang/lib/CodeGen/CGBuiltin.cpp:9068 +VecOps.push_back(llvm::ConstantInt::get( +EltTy, cast(Ops[I])->getZExtValue())); + else Constant doesn't imply ConstantInt. (For example, it could be the a

[PATCH] D103611: Correct the behavior of va_arg checking in C++

efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. LGTM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D103611/new/ https://reviews.llvm.org/D103611 ___ cfe-commits mailing list cfe-commi

[PATCH] D103082: [AArch64][SVE] Improve codegen for dupq SVE ACLE intrinsics

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/D103082/new/ https://reviews.llvm.org/D103082 ___

[PATCH] D103611: Correct the behavior of va_arg checking in C++

efriedma added inline comments. Comment at: clang/lib/Sema/SemaExpr.cpp:15783 + // test for typesAreCompatible() will already properly consider those to + // be compatible types. + if (Context.getLangOpts().CPlusPlus && !PromoteType.isNull() && Thi

[PATCH] D103611: Correct the behavior of va_arg checking in C++

efriedma added inline comments. Comment at: clang/lib/Sema/SemaExpr.cpp:15775 + if (Context.typesAreCompatible(PromoteType, UnderlyingType, + /*CompareUnqualified*/ true)) PromoteType = QualType(); If we're not go

[PATCH] D103495: [static initializers] Don't put ordered dynamic initializers of static variables into global_ctors

efriedma added a comment. Are there any existing optimizations that might be affected by this? In particular, I think GlobalOpt implicitly reorders functions in global_ctors. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D103495/new/ https://reviews.llvm.org/D103495 __

[PATCH] D103495: [static initializers] Emit global_ctors and global_dtors in reverse order when init_array is not used.

efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. LGTM. Making the order of constructors independent of UseInitArray seems obviously good in any case. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D103495/new/ https://reviews.ll

[PATCH] D103611: Correct the behavior of va_arg checking in C++

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

[PATCH] D103874: [IR] Rename the shufflevector's undef mask to poison

efriedma added a comment. I noted the cases where it looks like the undef->poison change might actually impact code using compiler intrinsic functions that have external specifications. The relevant specifications say the elements in question are "undefined", without really specifying what tha

[PATCH] D103874: [IR] Rename the shufflevector's undef mask to poison

efriedma added a comment. > In practice, the frozen element won't be used in most of the cases; the > middle-end's demanded elements analysis will trigger instcombine to almost > always remove the freeze. Well, in the cases it gets removed, it doesn't really matter what we use. It's likely if

[PATCH] D103958: [WIP] Support MustControl conditional control attribute

efriedma added a comment. I don't like using metadata like this. Dropping metadata should generally preserve the semantics of the code. > without resorting to inline assembly (which often results in poor codegen). Could you give an example of the resulting assembly with mustcontrol vs. this p

[PATCH] D103958: [WIP] Support MustControl conditional control attribute

efriedma added a comment. In D103958#2808861 , @efriedma wrote: >> without resorting to inline assembly (which often results in poor codegen). > > Could you give an example of the resulting assembly with mustcontrol vs. this > patch? Err, I mean, the re

[PATCH] D103958: [WIP] Support MustControl conditional control attribute

efriedma added a comment. In D103958#2808966 , @melver wrote: > In D103958#2808861 , @efriedma > wrote: > >> I don't like using metadata like this. Dropping metadata should generally >> preserve the semantics o

[PATCH] D103958: [WIP] Support MustControl conditional control attribute

efriedma added a comment. You could break `__builtin_load_with_control_dependency(x)` into something like `__builtin_control_dependency(READ_ONCE(x))`. I don't think any transforms will touch that in practice, even if it isn't theoretically sound. The rest of my suggestion still applies to th

[PATCH] D103958: [WIP] Support MustControl conditional control attribute

efriedma added a comment. > Defining the value used to establish a control dependency, e.g. the load > later writes depend on (kernel only defines writes to be ctrl-dependently > ordered). `[[mustcontrol]]` also has this problem. At the LLVM IR level, if just want to split the load from the co

[PATCH] D99439: Update @llvm.powi to handle different int sizes for the exponent

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/D99439/new/ https://reviews.llvm.org/D99439 _

[PATCH] D106959: [PowerPC] swdiv builtins for XL compatibility

efriedma added inline comments. Comment at: llvm/test/CodeGen/PowerPC/LowerCheckedFPArith.ll:36 +; CHECK-NEXT: %2 = fdiv fast float %0, %1 +; CHECK-NEXT: %3 = fcmp une float %2, %2 +; CHECK-NEXT: br i1 %3, label %swdiv_HWDIV, label %swdiv_MERGE quinnp wrote: >

[PATCH] D106753: ConvertUTF: Created wrapper convertUTF32ToUTF8String

efriedma added inline comments. Comment at: llvm/lib/Support/ConvertUTFWrapper.cpp:176 + // enough that we can fit a null terminator without reallocating. + Out.resize(SrcBytes.size() * UNI_MAX_UTF8_BYTES_PER_CODE_POINT + 1); + UTF8 *Dst = reinterpret_cast(&Out[0]); --

[PATCH] D106755: Extended format string checking to wprintf/wscanf

efriedma added subscribers: tahonermann, cor3ntin. efriedma added inline comments. Comment at: clang/lib/AST/Expr.cpp:1081 +ArrayRef AR(getTrailingObjects(), + getTrailingObjects() + getByteLength()); +if (llvm::convertUTF16ToUTF8String(AR, Output)) {

[PATCH] D106753: ConvertUTF: Created wrapper convertUTF32ToUTF8String

efriedma added a comment. As far as I can tell, the lastest version of the diff you uploaded still has the following issues that haven't been addressed: 1. The BOM handling is actually actively a problem if you're planning to use the interface to interpret wprintf format strings. We don't want

[PATCH] D106753: ConvertUTF: Created wrapper convertUTF32ToUTF8String

efriedma added a comment. If you're having trouble making Arcanist work correctly, you can always just upload "git diff" or "git show" output at https://reviews.llvm.org/differential/diff/create/ . CHANGES SINCE LAST ACTION https://reviews.llvm.org/D106753/new/ https://reviews.llvm.org/D106

[PATCH] D104854: Introduce intrinsic llvm.isnan

efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. LGTM. (Since there have been a bunch of reviewers involved, please give a few days before you merge.) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.l

[PATCH] D107116: [ConstantFold] Get rid of special cases for sizeof etc.

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG2a2847823f0d: [ConstantFold] Get rid of special cases for sizeof etc. (authored by efriedma). Herald added a project: clang. Herald added a subscribe

[PATCH] D107116: [ConstantFold] Get rid of special cases for sizeof etc.

efriedma added a comment. Just committed 6eb2ffba to fix a couple regression tests. Seems low-risk to cherry-pick to 13.x. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D107116/

[PATCH] D104854: Introduce intrinsic llvm.isnan

efriedma added a comment. A few of the AArch64 sequences don't look ideal, but that's not the fault of your patch. I'd like to see some test coverage for all the floating-point types (half, bfloat16, ppc_fp128). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://review

[PATCH] D105097: [clang][AArch64][SVE] Handle PRValue under VLAT <-> VLST cast

efriedma added inline comments. Comment at: clang/lib/CodeGen/CGExprScalar.cpp:2120 + } else +Addr = EmitLValue(E).getAddress(CGF); Addr = Builder.CreateElementBitCast(Addr, CGF.ConvertTypeForMem(DestTy)); I don't think it's legal to use EmitL

[PATCH] D105097: [clang][AArch64][SVE] Handle PRValue under VLAT <-> VLST cast

efriedma added inline comments. Comment at: clang/lib/CodeGen/CGExprScalar.cpp:2120 + } else +Addr = EmitLValue(E).getAddress(CGF); Addr = Builder.CreateElementBitCast(Addr, CGF.ConvertTypeForMem(DestTy)); junparser wrote: > junparser wrote: >

[PATCH] D105097: [clang][AArch64][SVE] Handle PRValue under VLAT <-> VLST cast

efriedma added inline comments. Comment at: clang/lib/CodeGen/CGExprScalar.cpp:2103 + if (const CallExpr *CE = dyn_cast(E)) +Ty = CE->getCallReturnType(CGF.getContext()); + I don't think we need to call getCallReturnType() here. A call that returns

[PATCH] D105097: [clang][AArch64][SVE] Handle PRValue under VLAT <-> VLST cast

efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. LGTM Comment at: clang/test/CodeGen/attr-arm-sve-vector-bits-globals.c:108 +// CHECK-128-NEXT:[[CASTFIXEDSVE:%.*]] = bitcast <2 x i8>* [[SAVED_VALUE]] to * +// CHECK

[PATCH] D94098: [Clang][AArch64] Inline assembly support for the ACLE type 'data512_t'.

efriedma added a comment. I'm confused what your goal here is, exactly. The point of allowing 512-bit inline asm operands is presumably to allow writing efficient code involving inline asm... but you're intentionally destroying any potential efficiency by forcing it to be passed/returned in me

[PATCH] D94098: [Clang][AArch64] Inline assembly support for the ACLE type 'data512_t'.

efriedma added a comment. The part I'm confused about is that you're forcing it to use "*r". At the IR level, LLVM handles something like `call void asm sideeffect "#$0", "r"([8 x i64] %c)` fine. You'll have to do a bit of work to teach clang to emit that, but it shouldn't be that hard. I th

[PATCH] D105360: [PowerPC] Fix popcntb XL Compat Builtin for 32bit

efriedma added inline comments. Comment at: llvm/lib/Target/PowerPC/PPCInstr64Bit.td:831 +// 64bit version of popcntb for 64bit sized unsigned long. +let isCodeGenOnly = 1 in +def POPCNTB8 : XForm_11<31, 122, (outs g8rc:$rA), (ins g8rc:$rS), I'm sort of confused

[PATCH] D94098: [Clang][AArch64] Inline assembly support for the ACLE type 'data512_t'.

efriedma added a comment. In D94098#2874976 , @labrinea wrote: > In D94098#2868751 , @efriedma wrote: > >> > > but in my honest opinion I don't see the benefit. The problem is, there isn't really any point to supp

[PATCH] D105360: [PowerPC] Fix popcntb XL Compat Builtin for 32bit

efriedma added inline comments. Comment at: llvm/lib/Target/PowerPC/PPCInstr64Bit.td:831 +// 64bit version of popcntb for 64bit sized unsigned long. +let isCodeGenOnly = 1 in +def POPCNTB8 : XForm_11<31, 122, (outs g8rc:$rA), (ins g8rc:$rS), nemanjai wrote: > ef

[PATCH] D98363: [Sema] Fold VLA types in compound literals to constant arrays.

efriedma created this revision. efriedma added reviewers: rsmith, erik.pilkington, aaron.ballman. efriedma requested review of this revision. Herald added a project: clang. Similar to variables with an initializer, this is never valid in standard C, so we can safely constant-fold as an extension.

[PATCH] D98363: [Sema] Fold VLA types in compound literals to constant arrays.

efriedma added inline comments. Comment at: clang/test/Sema/vla.c:134 + // expected-warning@+1{{variable length array folded to constant array as an extension}} + char (*a9)[] = (char[2][ksize]) {{1,2,3,4},{4,3,2,1}}; + aaron.ballman wrote: > Doesn't this viol

[PATCH] D98363: [Sema] Fold VLA types in compound literals to constant arrays.

efriedma added inline comments. Comment at: clang/test/Sema/vla.c:134 + // expected-warning@+1{{variable length array folded to constant array as an extension}} + char (*a9)[] = (char[2][ksize]) {{1,2,3,4},{4,3,2,1}}; + aaron.ballman wrote: > efriedma wrote: >

[PATCH] D136203: [ARM] Support -mexecute-only with -mlong-calls.

efriedma added inline comments. Comment at: llvm/test/CodeGen/Thumb2/thumb2-execute-only-long-calls.ll:19 +; STATIC-NEXT: movt[[REG0]], :upper16:.LCP1_0 +; STATIC-NEXT: ldr [[REG1:r[0-9]+]], [[[REG0]]] +; STATIC-NEXT: blx [[REG1]] Is there some rea

[PATCH] D136203: [ARM] Support -mexecute-only with -mlong-calls.

efriedma added a comment. > that I can redirect the function call at runtime, for dynamic linking purpose. Can you describe a little more what you're trying to do here? If you want to replace the implementation of an existing function at runtime, you'd be better off implementing the indirection

<    5   6   7   8   9   10   11   12   13   14   >