[PATCH] D132843: [RISCV] Ensure target features get passed to the LTO linker for RISC-V

efriedma added a comment. > So we need to keep ABI in somewhere and read that at LTO phase, the most > ideal place is the module flags. We already did that[6], but that comes with > a problem is it's too late to update datalayout when we start to read a > module, because LLVM will use

[PATCH] D125419: [Arm64EC 7/?] clang side of Arm64EC varargs ABI.

efriedma added inline comments. Comment at: clang/lib/CodeGen/TargetInfo.cpp:2457 +/*IsVectorCall=*/false, /*IsRegCall=*/false); + } + rjmccall wrote: > Hmm. Doesn't EC ABI lowering need to preserve this same state, or else > you'll get

[PATCH] D125419: [Arm64EC 7/?] clang side of Arm64EC varargs ABI.

efriedma updated this revision to Diff 457459. efriedma added a reviewer: rjmccall. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D125419/new/ https://reviews.llvm.org/D125419 Files: clang/lib/CodeGen/TargetInfo.cpp clang/test/CodeGen/arm64ec.c

[PATCH] D125418: [Arm64EC 6/?] Implement C/C++ mangling for Arm64EC function definitions.

efriedma updated this revision to Diff 457457. efriedma added a comment. Rebased Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D125418/new/ https://reviews.llvm.org/D125418 Files: clang/include/clang/AST/Mangle.h

[PATCH] D132568: [clang][Sema] check default argument promotions for printf

efriedma added inline comments. Comment at: clang/docs/ReleaseNotes.rst:141 +- Implemented `WG14 N2562 `_. + Clang will now consider default argument promotions in printf, and remove unnecessary warnings.

[PATCH] D132952: [Sema] disable -Wvla for function array parameters

efriedma added a comment. As a practical matter, there isn't any reason to force variably modified parameters to make a function variably modified. The types of parameters aren't visible in the caller of a function; we only check the compatibility for calls. Trying to treat `void (*)(int,

[PATCH] D132952: [Sema] disable -Wvla for function array parameters

efriedma added a comment. In D132952#3759226 , @aaron.ballman wrote: >> We could theoretically mess with the AST representation somehow, but I don't >> see any compelling reason to. We probably just want to emit the >> warn_vla_used diagnostic

[PATCH] D125418: [Arm64EC 6/?] Implement C/C++ mangling for Arm64EC function definitions.

efriedma added a comment. The reason struct returns require register shuffling is that AArch64 passes the sret pointer in x8 (i.e. RAX), but the x64 calling convention expects in in RCX (i.e. x0). Have you tried to see if the Microsoft-generated thunk actually works? I found at least one bug

[PATCH] D132952: [Sema] disable -Wvla for function array parameters

efriedma added a comment. > We have the VariableArrayType to represent a VLA type and I think we're using > that type when we should be using a variably modified type The clang AST for function signatures encodes two types: the type as written, and the type after promotion. Semantic analysis

[PATCH] D132848: [clang] Fix checking for emulated TLS in shouldAssumeDSOLocal in CodeGen

efriedma added a comment. I'm not really happy with the way this implicitly ties the implementation of this function to the implementation of TargetMachine::useEmulatedTLS(). I'd prefer to just make clang always explicitly compute whether EmullatedTLS is enabled, then always set

[PATCH] D132843: [RISCV] Ensure target features get passed to the LTO linker for RISC-V

efriedma added a comment. I'd prefer to use an approach with less moving parts: the bitcode should contain enough information to generate code without specifying anything on the command-line that wouldn't normally be passed to the linker. Among other issues, it's hard to understand the

[PATCH] D132877: Downgrade the UAX31 diagnostic to a warning which defaults to an error

efriedma added a comment. I'm not really a fan of the argument of "this was accepted as a DR, therefore new versions of clang have to reject code that old versions accepted". Regardless of what the C++ committee does, our commitment to our users is to ensure that code written against

[PATCH] D132877: Downgrade the UAX31 diagnostic to a warning which defaults to an error

efriedma added a comment. If we don't specifically call out the deprecation period in the diagnostic, usage will grow beyond what we expect. (Most people don't read the release notes; they'll just see it's possible to turn off the error message, and turn it off.) If we're okay with people

[PATCH] D125418: [Arm64EC 6/?] Implement C/C++ mangling for Arm64EC function definitions.

efriedma added a comment. There's no way the calling convention can change based on whether you're calling a function vs. a function pointer. I can't explain why MSVC is generating different code. I think we should just ignore it, at least for now. Repository: rG LLVM Github Monorepo

[PATCH] D132531: [AArch64] Reserve more physical registers

efriedma added a comment. > A reserved physical register doesn't mean it can't be used by > compiler/linker, it just means it can't be used by register allocator, see > the comments in TargetRegisterInfo::getReservedRegs(). The target feature to "reserve" a register is meant to be "don't use

[PATCH] D132275: [clang] Create alloca to pass into static lambda

efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. LGTM (We might want to consider messing with the rules for calling lambdas that don't capture anything, since there's no way to actually access it from the lambda body. But it's

[PATCH] D132275: [clang] Reset some attributed calling lambda

efriedma added a comment. In D132275#3740802 , @vitalybuka wrote: > In D132275#3740479 , @efriedma > wrote: > >> Among other things, we could use a definition from a different module. > > Is this a thing, I

[PATCH] D132275: [clang] Reset some attributed calling lambda

efriedma added a comment. Given that we mark up lambdas with the requirement that the pointer is nonnull/noundef/dereferenceable, we can't fix that by just messing with the attributes. Among other things, we could use a definition from a different module. Either we relax the requirements

[PATCH] D119296: KCFI sanitizer

efriedma added inline comments. Comment at: clang/docs/ControlFlowIntegrity.rst:319 +cross-DSO function address equality. These properties make KCFI easier to +adopt in low-level software. KCFI is limited to indirect call checking only, +and isn't compatible with executable-only

[PATCH] D127233: [CodeGen] Sort llvm.global_ctors by lexing order before emission

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

[PATCH] D127233: [CodeGen] Sort llvm.global_ctors by lexing order before emission

efriedma added inline comments. Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:580 +I = DelayedCXXInitPosition.find(D); +unsigned LexOrder = I == DelayedCXXInitPosition.end() ? ~0U : I->second; +AddGlobalCtor(Fn, 65535, LexOrder, COMDATKey); ychen

[PATCH] D127233: [CodeGen] Sort llvm.global_ctors by lexing order before emission

efriedma added inline comments. Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:580 +I = DelayedCXXInitPosition.find(D); +unsigned LexOrder = I == DelayedCXXInitPosition.end() ? ~0U : I->second; +AddGlobalCtor(Fn, 65535, LexOrder, COMDATKey); ychen

[PATCH] D127233: [CodeGen] Sort llvm.global_ctors by lexical order before emission

efriedma added inline comments. Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:580 +I = DelayedCXXInitPosition.find(D); +unsigned LexOrder = I == DelayedCXXInitPosition.end() ? ~0U : I->second; +AddGlobalCtor(Fn, 65535, LexOrder, COMDATKey); ychen

[PATCH] D131980: [Passes] Don't run tail-call-elim in -O1

efriedma added a comment. If you're specifically concerned about sibcall (call->jmp) optimization in the backend, it might be better to adjust the backend to avoid sibcalls at -O1, as opposed to messing with optimization passes. (i.e. make -fno-optimize-sibling-calls the default at -O1.)

[PATCH] D131980: [Passes] Don't run tail-call-elim in -O1

efriedma added a comment. Are you concerned about tail-call marking, or the recursive call->loop transform? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D131980/new/ https://reviews.llvm.org/D131980

[PATCH] D127233: [CodeGen] Sort llvm.global_ctors by lexical order before emission

efriedma added a comment. > is this an ABI breaking change It only affects the order of initialization within a file, so it doesn't really have any implications for the binary ABI. It might break code, of course, but that's a different issue. If we want a flag to maintain the old behavior,

[PATCH] D125418: [Arm64EC 6/?] Implement C/C++ mangling for Arm64EC function definitions.

efriedma added inline comments. Comment at: clang/lib/CodeGen/CodeGenModule.cpp:5128 +// to the function itself; it points to a stub for the compiler. +// FIXME: We also need to emit an entry thunk. +SmallString<256> MangledName; bcl5980 wrote: >

[PATCH] D125418: [Arm64EC 6/?] Implement C/C++ mangling for Arm64EC function definitions.

efriedma added inline comments. Comment at: clang/lib/CodeGen/CodeGenModule.cpp:5128 +// to the function itself; it points to a stub for the compiler. +// FIXME: We also need to emit an entry thunk. +SmallString<256> MangledName; bcl5980 wrote: >

[PATCH] D131194: [C++20] Fix crash-on-valid with consteval temporary construction through list initialization

efriedma accepted this revision as: efriedma. efriedma added a comment. This revision is now accepted and ready to land. LGTM Comment at: clang/lib/CodeGen/CGExprConstant.cpp:1402 +RetType = CGM.getContext().getLValueReferenceType(RetType); + assert(!RetType.isNull() &&

[PATCH] D125418: [Arm64EC 6/?] Implement C/C++ mangling for Arm64EC function definitions.

efriedma added inline comments. Comment at: clang/lib/CodeGen/CodeGenModule.cpp:5128 +// to the function itself; it points to a stub for the compiler. +// FIXME: We also need to emit an entry thunk. +SmallString<256> MangledName; bcl5980 wrote: >

[PATCH] D123627: Correctly diagnose prototype redeclaration errors in C

efriedma added a comment. If the declaration we're redeclaring is a builtin, should the diagnostic be in the "-Wincompatible-library-redeclaration" warning group? With this patch, we treat redefinitions of builtins without a prototype differently from redefinitions with a prototype, for

[PATCH] D131194: [C++20] Fix crash-on-valid with consteval temporary construction through list initialization

efriedma added a comment. Instead of trying to dig through subexpressions of the ConstantExpr to try to infer the type we need, can we just compute it directly? QualType RetType = CE->getType(); if (CE->isGLValue()) RetType = CGM.getContext().getLValueReferenceType(RetType);

[PATCH] D130907: [Clang] Use pragma FENV_ROUND to reset rounding mode settings.

efriedma added inline comments. Comment at: clang/lib/Sema/SemaStmt.cpp:398 +// If dynamic rounding mode is specified in absence of FENV_ACCESS, treat it +// as setting default rounding mode. +FpPragmaStack.CurrentValue.clearConstRoundingModeOverride();

[PATCH] D131143: [Clang] Interaction of FP pragmas and function template instantiation

efriedma added a comment. > In the case is explicit instantiation there is apparent connection > between a point in source code and instantiated function. It can > support interaction of pragmas that act in that point. For example, in > the code: > > #pragma STDC FENV_ROUND FE_DOWNWARD >

[PATCH] D129131: Remove uses of llvm_shutdown

efriedma added inline comments. Comment at: polly/lib/External/isl/interface/extract_interface.cc:590 delete Clang; - llvm::llvm_shutdown(); nhaehnle wrote: > Meinersbur wrote: > > This file is imported from the upstream project > >

[PATCH] D130577: clang-driver: use llvm_fast_shutdown

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

[PATCH] D129118: CommandLine: add and use cl::SubCommand::get{All,TopLevel}

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

[PATCH] D130576: ManagedStatic: remove from ASTMatchersInternal.h

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

[PATCH] D130574: ClangLinkerWrapper: explicitly #include

efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. LGTM You don't really need pre-commit review for a patch that just adds an include, though. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D130575: clang: include ManagedStatic.h for llvm_shutdown

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

[PATCH] D129881: [C] Strengthen -Wint-conversion to default to an error

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

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

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

[PATCH] D125418: [Arm64EC 6/?] Implement C/C++ mangling for Arm64EC function definitions.

efriedma added inline comments. Comment at: clang/lib/CodeGen/CodeGenModule.cpp:5128 +// to the function itself; it points to a stub for the compiler. +// FIXME: We also need to emit an entry thunk. +SmallString<256> MangledName; bcl5980 wrote: > A

[PATCH] D129881: [C] Strengthen -Wint-conversion to default to an error

efriedma added a comment. Given this is an on-by-default warning already, I don't expect changing it to an error by default to have much impact in practice. Changing it fine. Comment at: clang/lib/Sema/SemaExprCXX.cpp:8483 +if ((SemaRef.getLangOpts().CPlusPlus &&

[PATCH] D129802: [DRAFT] Implementing new atomic orderings in LLVM and generate barriers for legacy __sync builtins. Support corresponding memory model in outline atomics as well.

efriedma added a comment. `atomicrmw add [...] sync_seq_cst` is supposed to be equivalent to `fence seq_cst; atomicrmw add seq_cst; fence seq_cst`, I think. The extra fences aren't necessary for most uses of `__sync_*`, but the difference is theoretically visible for exotic synchronization

[PATCH] D129476: [AArch64][SVE] Prefer SIMD variant of clast[ab]

efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. Please update the comment, then LGTM Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:801 +

[PATCH] D129476: [AArch64][SVE] Prefer SIMD variant of clast[ab]

efriedma added a comment. Are you saying that it's faster to use clasta targeting a float register, then move the result to an integer register, rather than use the integer form directly? Or is the issue just that we want to split the operations in case we can simplify the resulting bitcast?

[PATCH] D126956: [tbaa] Handle base classes in struct tbaa

efriedma added inline comments. Comment at: clang/lib/CodeGen/CodeGenTBAA.cpp:366 + // so may differ from declaration order. In particular, Itanium ABI will + // allocate a primary base first. + llvm::sort(Fields, Maybe add a comment explaining

[PATCH] D128501: [CodeGen] Make uninitialized Lvalue bit-field stores poison compatible

efriedma added a comment. > Is that seen as a defect or as for some reason desirable? Because I worry > that optimizer people are talking themselves into something that would be a > truly beautiful model if only there were a frontend that could emit code for > it. > > Doesn't this make it e.g.

[PATCH] D128501: [CodeGen] Make uninitialized Lvalue bit-field stores poison compatible

efriedma added a comment. > Is there no way in LLVM to load partially-initialized memory and then freeze > it that will preserve the parts of the value that are initialized? Because > that seems like something that LLVM needs to be providing. It's currently possible to `load <32 x i1>`, freeze

[PATCH] D128501: [CodeGen] Make uninitialized Lvalue bit-field stores poison compatible

efriedma added a comment. > This only matters if the IRs are linked together with IPO. Otherwise, at > object level it doesn't really matter. Right. > Do you think we can get away by just documenting the incompatibility of doing > IPO with files compiled with different

[PATCH] D128501: [CodeGen] Make uninitialized Lvalue bit-field stores poison compatible

efriedma added a comment. > No, you can still link those. There's no ABI change nor any interference at > IR level. The scenario I was thinking of with -ffine-grained-bitfield-accesses is something like the following: File A: struct X { int a : 8; int b : 24; }; void f(struct X*); int

[PATCH] D126864: [clang] Introduce -fstrict-flex-arrays= for stricter handling of flexible arrays

efriedma added a comment. I suspect the refactoring in the latest version of the patch accidentally made UBSan a bit more strict by default. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D126864/new/ https://reviews.llvm.org/D126864 ___

[PATCH] D128501: [CodeGen] Make uninitialized Lvalue bit-field stores poison compatible

efriedma added a comment. Under this scheme, is it illegal to link together object files built with -ffine-grained-bitfield-accesses and object files built with -fno-fine-grained-bitfield-accesses? Do we want to add a temporary option to control this, to make it easier for people to benchmark

[PATCH] D128482: [clang codegen] Add dso_local/hidden/etc. markings to VTT declarations

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGe11bf8de729a: [clang codegen] Add dso_local/hidden/etc. markings to VTT declarations (authored by efriedma). Repository: rG LLVM Github Monorepo

[PATCH] D128482: [clang codegen] Add dso_local/hidden/etc. markings to VTT declarations

efriedma created this revision. efriedma added reviewers: rjmccall, MaskRay. Herald added a subscriber: StephenFan. Herald added a project: All. efriedma requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. We were marking definitions, but not

[PATCH] D125094: [ARM][Thumb] Command-line option to ensure AAPCS compliant Frame Records

efriedma accepted this revision. efriedma added a comment. LGTM with one minor comment (feel free to ignore if you disagree) Comment at: llvm/lib/Target/ARM/Thumb1FrameLowering.cpp:958 +static void popRegsFromStack(MachineBasicBlock , +

[PATCH] D126364: Fix interaction of pragma FENV_ACCESS with other pragmas

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

[PATCH] D126864: [clang] Introduce -fstrict-flex-arrays for stricter handling of flexible arrays

efriedma added a comment. Maybe we should document that zero-size arrays aren't treated as flexible arrays, since that's what the zero-size array extension was originally designed for. Looks fine otherwise. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D126864/new/

[PATCH] D127460: Rename GCCBuiltin into ClangBuiltin

efriedma added a comment. No other comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D127460/new/ https://reviews.llvm.org/D127460 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D127460: Rename GCCBuiltin into ClangBuiltin

efriedma added a comment. Why are you messing with test_demangle.pass.cpp? The demangler doesn't care what symbols we actually define in LLVM... it's just a bunch of hardcoded testcases. So it doesn't matter if it continues to refer to GCCBuiltin. Other changes look fine. Repository: rG

[PATCH] D127684: [NFC] Use `https` instead of `http` in the urls

efriedma added a comment. A couple notes skimming the changes: 1. If you're going to update URLs, please verify the new URLs are actually valid. 2. Some of the "URLs" you're updating aren't actually URLs. For example, the `xmlns` attribute in SVG files must be the exact string written in the

[PATCH] D125094: [ARM][Thumb] Command-line option to ensure AAPCS compliant Frame Records

efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. LGTM The Thumb1 changes here are complicated, so I'm not sure we found all the issues, but you fixed all the issues I spotted. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D126364: Fix interaction of pragma FENV_ACCESS with other pragmas

efriedma added inline comments. Comment at: clang/include/clang/Basic/LangOptions.h:622 setFPContractMode(LangOptions::FPM_Off); setRoundingMode(static_cast(LangOptions::FPR_ToNearest)); setFPExceptionMode(LangOptions::FPE_Ignore); sepavloff

[PATCH] D125094: [ARM][Thumb] Command-line option to ensure AAPCS compliant Frame Records

efriedma added inline comments. Comment at: llvm/test/CodeGen/Thumb/frame-access.ll:335 +; CHECK-FP-AAPCS: mov r1, r11 +; CHECK-FP-AAPCS: ldr r0, [r0, r1] +; CHECK: bl i This sequence requires, in general, scavenging two registers. I'm not sure we can do that

[PATCH] D126864: [clang] Introduce -fstrict-flex-arrays for stricter handling of flexible arrays

efriedma added a comment. > As for SOCK_MAXADDRLEN, that's a horrid hack, and the definition of struct > sockaddr needs to change. :) Do we want some builtin define so headers can check if we're in -fstrict-flex-arrays mode? It might be hard to mess with the definitions otherwise. CHANGES

[PATCH] D126364: Fix interaction of pragma FENV_ACCESS with other pragmas

efriedma added inline comments. Comment at: clang/include/clang/Basic/LangOptions.h:622 setFPContractMode(LangOptions::FPM_Off); setRoundingMode(static_cast(LangOptions::FPR_ToNearest)); setFPExceptionMode(LangOptions::FPE_Ignore); sepavloff

[PATCH] D126364: Fix interaction of pragma FENV_ACCESS with other pragmas

efriedma added inline comments. Comment at: clang/include/clang/Basic/LangOptions.h:622 setFPContractMode(LangOptions::FPM_Off); setRoundingMode(static_cast(LangOptions::FPR_ToNearest)); setFPExceptionMode(LangOptions::FPE_Ignore); sepavloff

[PATCH] D126871: [clang] [MinGW] Use SEH for unwind info on ARM by default

efriedma added a comment. Funclets are necessary for both C++ and SEH exception handling, yes. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D126871/new/ https://reviews.llvm.org/D126871 ___ cfe-commits

[PATCH] D126871: [clang] [MinGW] Use SEH for unwind info on ARM by default

efriedma added a comment. > Btw, thanks a lot for your reviews, you've spent a lot of time on them! You're welcome. > But for MSVC style (C++ exceptions and __try/__except) there's still a couple > backend things that need to be implemented. Do you happen to know roughly how > much effort

[PATCH] D126364: Fix interaction of pragma FENV_ACCESS with other pragmas

efriedma added inline comments. Comment at: clang/include/clang/Basic/LangOptions.h:622 setFPContractMode(LangOptions::FPM_Off); setRoundingMode(static_cast(LangOptions::FPR_ToNearest)); setFPExceptionMode(LangOptions::FPE_Ignore); sepavloff

[PATCH] D126364: Fix interaction of pragma FENV_ACCESS with other pragmas

efriedma added inline comments. Comment at: clang/include/clang/Basic/LangOptions.h:622 setFPContractMode(LangOptions::FPM_Off); setRoundingMode(static_cast(LangOptions::FPR_ToNearest)); setFPExceptionMode(LangOptions::FPE_Ignore); I'm suggesting

[PATCH] D126364: Fix interaction of pragma FENV_ACCESS with other pragmas

efriedma added a comment. In D126364#3560984 , @sepavloff wrote: > In D126364#3560877 , @efriedma > wrote: > >> Shouldn't the rounding mode be FE_DYNAMIC by default? > > According to the standard it must be

[PATCH] D126871: [clang] [MinGW] Use SEH for unwind info on ARM by default

efriedma added a comment. Oh, just remembered, we should probably release-note this. (You can post that as a followup.) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D126871/new/ https://reviews.llvm.org/D126871

[PATCH] D126364: Fix interaction of pragma FENV_ACCESS with other pragmas

efriedma added a comment. Shouldn't the rounding mode be FE_DYNAMIC by default? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D126364/new/ https://reviews.llvm.org/D126364 ___ cfe-commits mailing list

[PATCH] D126871: [clang] [MinGW] Use SEH for unwind info on ARM by default

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

[PATCH] D126364: Fix interaction of pragma FENV_ACCESS with other pragmas

efriedma added a comment. Given we have getEffectiveRoundingMode(), I think the calls to setRoundingModeOverride shouldn't be necessary? And if we drop those calls, we can also drop the IsRoundingModeSet variable. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D126864: [clang] Introduce -fstrict-flex-arrays for stricter handling of flexible arrays

efriedma added a comment. Probably IsTailPaddedMemberArray() in SemaChecking.cpp and isFlexibleArrayMemberExpr() in CGExpr.cpp should also check for this flag. Not sure if there's anything else that does similar checks; the static analyzer has its own flag

[PATCH] D126816: Only issue warning for subtraction involving null pointers on live code paths

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

[PATCH] D126764: [clang] [ARM] Add __builtin_sponentry like on aarch64

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

[PATCH] D126364: Fix interaction of pragma FENV_ACCESS with other pragmas

efriedma added a comment. > Constrained intrinsics don't change the rounding mode. The standard text for FENV_ROUND requires that when we see a floating point operation, we have to perform it using the specified rounding mode. "floating-point operators [...] shall be evaluated according to

[PATCH] D126364: Fix interaction of pragma FENV_ACCESS with other pragmas

efriedma added a comment. The way I see it, there are two possibilities here: 1. In Sema, we have two rounding modes that correspond to FE_DYNAMIC: llvm::RoundingMode::Dynamic, and llvm::RoundingMode::NearestTiesToEven, plus some boolean to indicate whether the user actually explicitly

[PATCH] D126364: Fix interaction of pragma FENV_ACCESS with other pragmas

efriedma added a comment. > setRoundingMode definitely should not call getAllowFEnvAccess() and it does > not Yes, it does? // C2x: 7.6.2p3 If the FE_DYNAMIC mode is specified and FENV_ACCESS is "off", // the translator may assume that the default rounding mode is in effect. if (FPR ==

[PATCH] D125094: [ARM][Thumb] Command-line option to ensure AAPCS compliant Frame Records

efriedma added inline comments. Comment at: llvm/lib/Target/ARM/ARMFrameLowering.cpp:732 + // AAPCS requires use of R11, and PACBTI gets in the way of regular pushes, + // so FP ends up on area two. if (HasFP) { I guess this is related to this patch because

[PATCH] D116088: [compiler-rt] Implement ARM atomic operations for architectures without SMP support

efriedma added a comment. ldr+op+swp still isn't atomic. For each point in the code, please try the exercise of "what if my code is interrupted here"? The only way to use swp to implement general atomic operations is to use a separate spinlock. Repository: rG LLVM Github Monorepo CHANGES

[PATCH] D126364: Fix interaction of pragma FENV_ACCESS with other pragmas

efriedma added a comment. I mean that ActOnPragmaFEnvAccess shouldn't call hasRoundingModeOverride()/setRoundingModeOverride(), and setRoundingMode shouldn't call getAllowFEnvAccess(). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D126364/new/

[PATCH] D126494: [clang] avoid assert due to device treated long double as double

efriedma added a comment. This doesn't make sense to me. If clang and LLVM disagree about the layout of a type, you are going to run into trouble. If you haven't yet, it's a matter of time. Looking at AMDGPUTargetInfo::setAuxTarget, maybe LongDoubleFormat is somehow inconsistent with

[PATCH] D126364: Fix interaction of pragma FENV_ACCESS with other pragmas

efriedma added a comment. For FENV_ROUND, I think we should try to stick to the standard as closely as possible in Sema; since the standard models FENV_ROUND as a separate state, Sema should also model it as a separate state. If CodeGen decides it needs to modify the FP environment to

[PATCH] D126364: Fix interaction of pragma FENV_ACCESS with other pragmas

efriedma added a comment. Could you lay out the expected interaction between "STDC FENV_ACCESS", "clang fp exceptions", "float_control", and "fenv_access"? If there's some way to map everything to "#pragma clang fp", please lay that out; if that isn't possible, please explain why. As far as

[PATCH] D116088: [compiler-rt] Implement ARM atomic operations for architectures without SMP support

efriedma added a comment. In D116088#3534100 , @kpdev42 wrote: > 1. Sets memory barrier > 2. Calls atomic load from memory location > 3. Modifies data > 4. Calls atomic store to memory location > 5. Checks that operation is consistent, if not goes to

[PATCH] D126024: [MSVC, ARM64] Add __readx18 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/D126024/new/ https://reviews.llvm.org/D126024

[PATCH] D126023: [MSVC, ARM64] Add __writex18 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/D126023/new/ https://reviews.llvm.org/D126023

[PATCH] D126023: [MSVC, ARM64] Add __writex18 intrinsics

efriedma added inline comments. Comment at: clang/lib/CodeGen/CGBuiltin.cpp:9977 +StoreInst *Store = Builder.CreateAlignedStore( +Val, Ptr, getContext().getTypeAlignInChars(E->getType())); +return Store; I think I'd prefer just

[PATCH] D125094: [ARM][Thumb] Command-line option to ensure AAPCS compliant Frame Records

efriedma added inline comments. Comment at: llvm/lib/Target/ARM/ARMFrameLowering.cpp:1863 // to combine multiple loads / stores. - bool CanEliminateFrame = true; + bool CanEliminateFrame = !requiresAAPCSFrameRecord(MF); bool CS1Spilled = false; Should

[PATCH] D126023: [MSVC, ARM64] Add __writex18 intrinsics

efriedma added a comment. `uxtw #3` makes me think you're not generating the right code... the zero-extension hopefully doesn't matter, but the shift is significant. Maybe should be generating `getelementptr i8`? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D123649: Allow flexible array initialization in C++.

efriedma added inline comments. Comment at: clang/lib/CodeGen/CodeGenModule.cpp:4639 + getDataLayout().getTypeAllocSize(Init->getType())); + assert(VarSize == CstSize && "Emitted constant has unexpected size"); +#endif ahatanak wrote: > efriedma

[PATCH] D125291: Introduce @llvm.threadlocal.address intrinsic to access TLS variable (1/3)

efriedma added a comment. I don't really understand how this is supposed to interact with D125292 ; even if you strip the readnone attribute from the call instruction, we'll still treat a call to the intrinsic as readnone. I think I'd prefer to lower the

[PATCH] D125094: [ARM][Thumb] Command-line option to ensure AAPCS compliant Frame Records

efriedma added inline comments. Comment at: llvm/lib/Target/ARM/ARMFrameLowering.cpp:1844 +for (const auto : MBB) + if (MI.getOpcode() == ARM::tSTRspi || MI.getOpcode() == ARM::tSTRi) +for (const auto : MI.operands()) How do you end up with

[PATCH] D116088: [compiler-rt] Implement ARM atomic operations for architectures without SMP support

efriedma added a comment. On a target that doesn't support SMP, you don't need memory barriers, sure. (I think we'd want a CMake flag to explicitly assert that you're only going to run the code on chips without SMP.) That doesn't really solve your issue, though. To implement atomic

[PATCH] D125557: [APInt] Remove all uses of zextOrSelf, sextOrSelf and truncOrSelf

efriedma added inline comments. Comment at: llvm/lib/IR/ConstantRange.cpp:724 auto BW = getBitWidth(); -APInt Min = APInt::getMinValue(BW).zextOrSelf(ResultBitWidth); -APInt Max = APInt::getMaxValue(BW).zextOrSelf(ResultBitWidth); +APInt Min =

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