[PATCH] D107882: BPF: Enable frontend constant folding for VLA size

efriedma added a comment. I'd prefer not to mess with the AST if we don't need to; more differences between targets make it harder to understand any issues that come up. BPF-flavored C already has enough weird differences without adding unnecessary changes. If all you need is to avoid unneces

[PATCH] D101759: [PowerPC] Scalar IBM MASS library conversion pass

efriedma added a comment. > I agree. I think the problem is that this patch is trying to decide on a > global lowering strategy for llvm.* math intrinsics in > llvm/lib/Target/PowerPC/PPCISelLowering.cpp but such global decision making > does not go well with finer granularity of fast-math flag

[PATCH] D101759: [PowerPC] Scalar IBM MASS library conversion pass

efriedma added a comment. errno handling for math library functions is a mess. Currently, we don't model it properly; we just mark the calls "readnone" and hope for the best. If you don't want to fix that, just check for readnone for now. I don't think we want to be querying function attribut

[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] 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] 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] 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] 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] 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 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] 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] 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 A "fast" fdiv n

[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] D105516: [clang][PassManager] Add -falways-mem2reg to run mem2reg at -O0

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

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

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/D94098/new/ https://reviews.llvm.org/D94098 ___ cfe-commits mailing list cfe-commits

[PATCH] D104854: Introduce intrinsic llvm.isnan

efriedma added a comment. In D104854#2904826 , @sepavloff wrote: > In D104854#2886328 , @efriedma > wrote: > >>> The options '-ffast-math' and '-fno-honor-nans' imply that FP operation >>> operands are never NaNs

[PATCH] D105728: [clang][Codegen] Directly lower `(*((volatile int *)(0))) = 0;` into a `call void @llvm.trap()`

efriedma added a comment. gcc apparently lowers this sort of construct to a volatile load/store followed by a trap. Not sure where the trap is coming from. I'm not sure this solves any problem that really needs to be solved. LLVM isn't going to optimize out a volatile load/store to null. Wel

[PATCH] D106753: ConvertUTF: Created wrapper convertUTF32ToUTF8String

efriedma added a comment. The buildbot seems to think your new unittests are broken. Not sure why. (You can run just the unittests with "ninja check-llvm-unit".) Comment at: llvm/include/llvm/Support/ConvertUTF.h:127 +#define UNI_UTF32_BYTE_ORDER_MARK_NATIVE 0xFEFF +#def

[PATCH] D106753: ConvertUTF: Created wrapper convertUTF32ToUTF8String

efriedma added a comment. Please fix clang-format issues. (You can use the clang-format-diff.py script.) Comment at: llvm/lib/Support/ConvertUTFWrapper.cpp:154 + // Error out on an uneven byte count. + if (SrcBytes.size() % 2) +return false; Wrong consta

[PATCH] D106721: [AArch64] Implemnt MSVC __mulh and __umulh builtins and corresponding IR level intrinsics

efriedma added a comment. We won't call compiler-rt for i128 multiply on aarch64. It's not worthwhile under any circumstances. Pattern-matching 64/64->128 multiply in SelectionDAG legalization has been reliable in practice, as far as I know. And even it misses due to weird behavior in instco

[PATCH] D103426: Clang: Extend format string checking to wprintf/wscanf

efriedma added a comment. This should split into three patches: 1. The ConvertUTFWrapper patch. 2. The patch to add format warnings for wprintf/wscanf/etc. 3. The patch to add the %l16/%l32 format modifiers. Comment at: clang/lib/AST/Expr.cpp:1091 + if (llvm::convertUTF32

[PATCH] D106721: [AArch64] Implemnt MSVC __mulh and __umulh builtins and corresponding IR level intrinsics

efriedma added a comment. Do we need LLVM intrinsics for these? For the x86 equivalents, we just generate `mul i128`. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D106721/new/ https://reviews.llvm.org/D106721

[PATCH] D106701: [clang] Add -falign-loops=N where N is a power of 2

efriedma added a comment. Can we hook this up to a LLVM IR function attribute, instead of making it a codegen flag? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D106701/new/ https://reviews.llvm.org/D106701 ___

[PATCH] D106333: [AArch64][SVE] Handle svbool_t VLST <-> VLAT/GNUT conversion

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

[PATCH] D106333: [AArch64][SVE] Handle svbool_t VLST <-> VLAT/GNUT conversion

efriedma added a comment. Can you add a helper somewhere for the `BT->getKind() == BuiltinType::SveBool ? getLangOpts().ArmSveVectorBits / getCharWidth() : getLangOpts().ArmSveVectorBits)` pattern? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D106

[PATCH] D105516: [clang][PassManager] Add -falways-mem2reg to run mem2reg at -O0

efriedma added a comment. The part I'm most uncomfortable with is sticking "mem2reg" in a public, documented driver option. I don't want to promise that the mem2reg pass will exist forever. We should be focused on making sure the options we add are stable, and compose effectively, not just be

[PATCH] D105516: [clang][PassManager] Add -falways-mem2reg to run mem2reg at -O0

efriedma added a comment. I think it would be better to focus on making -O1 more usable for this sort of purpose, rather than introduce -O0.5. I mean, there's a lot of wiggle-room between -O0 and -O2, but I don't think it makes sense to add a driver option that promises to run exactly one opti

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

efriedma added a comment. In D94098#2886319 , @labrinea wrote: > Firstly, the information that the load/store comes from an inline asm operand > gets lost by the time the SelectionDAG processes those nodes, and so we > cannot use a target hook to select

[PATCH] D104854: Introduce intrinsic llvm.isnan

efriedma added a comment. > The options '-ffast-math' and '-fno-honor-nans' imply that FP operation > operands are never NaNs. This assumption however should not be applied > to the functions that check FP number properties, including 'isnan'. If > such function returns expected result instead of

[PATCH] D106030: [Clang] add support for error+warning fn attrs

efriedma added a comment. In D106030#2878897 , @arsenm wrote: > Adding something to the IR for the sole purpose of producing a diagnostic > feels really weird. I'm not sure I see why the frontend can't see this > attribute and directly warn In C++, you

[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] 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), I'm sort of confused

[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] 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] 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] 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 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: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] 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] 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] 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] 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. 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. 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. 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] 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] 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] 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] 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] 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] 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] 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] 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 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 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 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 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] 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] 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] 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] 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] 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] 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] 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] 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] 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] 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] 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] 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] 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] 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] D102015: [clang CodeGen] Don't crash on large atomic function parameter.

efriedma added a comment. > Using _Atomic for C structs with non-trivially destructible fields currently > doesn't work, so maybe that should just be disallowed. I'd prefer to disallow _Atomic on all types that aren't trivially destructible, yes. Not impossible to support, of course, but I don

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

efriedma added a comment. Made a couple suggestions to make this easier to review. The test changes you've made so far seem reasonable. Is there some specific section of the code you want feedback on? Comment at: clang/include/clang/Basic/Thunk.h:1 +//===- Thunk.h - Decla

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

efriedma updated this revision to Diff 344193. efriedma added a comment. Use isRecordType() instead of checking for an atomic type. Fix the caller side as well. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D102015/new/ https://reviews.llvm.org/D1

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

efriedma added a comment. In D102015#2748441 , @efriedma wrote: >> ...I'm confused about why this code is doing what it's doing with cleanups, >> though. Why does it only apply when the parameter is indirect? I believe >> `isParamDestroyedInCallee()`

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

efriedma added a comment. In D102015#2743634 , @rjmccall wrote: > Objective-C object types also have aggregate evaluation kind. Those can only > be used as value types in an old, fragile ObjC ABI, but I believe we haven't > yet formally decided to remo

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

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/D98798/new/ https://reviews.llvm.org/D98798 ___ cfe-commits mailing list cfe-commits

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

efriedma created this revision. efriedma added reviewers: rsmith, ahatanak, rjmccall. Herald added a subscriber: jfb. efriedma requested review of this revision. Herald added a project: clang. I wouldn't recommend writing code like the testcase; a function parameter isn't atomic, so using an atom

[PATCH] D101873: [clang] Support clang -fpic -fno-semantic-interposition for AArch64

efriedma added a comment. In D101873#2741903 , @peter.smith wrote: > Looking at the gist I've got one concern for AArch64 and Arm. The ABI relies > on thunks which are only defined for symbols of type STT_FUNC. Changing > branches to STT_FUNC to STT_SE

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

efriedma added inline comments. Comment at: clang/lib/Sema/SemaExpr.cpp:10723-10733 + bool LHSIsNullPtr = LHS.get()->IgnoreParenCasts()->isNullPointerConstant( + Context, Expr::NPC_ValueDependentIsNotNull); + bool RHSIsNullPtr = RHS.get()->IgnoreParenCasts()->i

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

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGdc1ab590a052: [Sema] Fold VLA types in compound literals to constant arrays. (authored by efriedma). Repository: rG LLVM Github Monorepo CHANGES

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

efriedma added inline comments. Comment at: clang/lib/Sema/SemaExpr.cpp:10724 + // Subtracting from a null pointer should produce a warning. + if (LHS.get()->IgnoreParenCasts()->isNullPointerConstant( + Context, Expr::NPC_ValueDependentIsNotNull)) -

[PATCH] D99675: RFC [llvm][clang] Create new intrinsic llvm.arith.fence to control FP optimization at expression level

efriedma added a comment. In D99675#2672138 , @kbsmith1 wrote: > In D99675#2671924 , @efriedma wrote: > >> How is llvm.arith.fence() different from using "freeze" on a floating-point >> value? The goal isn't reall

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

efriedma added a comment. Please add a test for `(char*)0-(char*)0`. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D98798/new/ https://reviews.llvm.org/D98798 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-b

[PATCH] D99675: RFC [llvm][clang] Create new intrinsic llvm.arith.fence to control FP optimization at expression level

efriedma added a comment. > The expression “llvm.arith.fence(a * b) + c” means that “a * b” must happen > before “+ c” and FMA guarantees that, but to prevent later optimizations from > unpacking the FMA the correct transformation needs to be: > > llvm.arith.fence(a * b) + c → llvm.arith.fence

[PATCH] D99791: [CGCall] Annotate pointer argument with alignment

efriedma added a comment. This feels scary: the C standard technically allows this, but we haven't done it in the past, and it could break otherwise functioning code. (We've only assumed alignment about pointers that are dereferenced/dereferenceable.) For D99790

[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] 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] 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 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] 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] 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] 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 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] 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] 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] 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] 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 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, +

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