Page MenuHomePhabricator

jfb (JF Bastien)
User

Projects

User does not belong to any projects.

User Details

User Since
Jan 27 2014, 9:36 AM (341 w, 1 d)

Recent Activity

Today

jfb added a comment to D79279: Add overloaded versions of builtin mem* functions.

I think it would be reasonable in general to guarantee that our __builtin_ functions have contracts at least as wide as the underlying C function, but allow them to have extensions, and to keep the plain C functions unextended. I had actually thought we already did that in more cases than we do (but perhaps I was thinking about the LLVM math intrinsics that guarantee to not set errno). That would mean that a C standard library implementation is still free to #define foo(x,y,z) __builtin_foo(x,y,z), but if they do, they may pick up extensions.

Tue, Aug 11, 11:17 AM · Restricted Project, Restricted Project

Yesterday

jfb added a comment to D85691: lld: link libatomic if needed for Timer.
In D85691#2208323, @jfb wrote:

You're probably better off putting the std::chrono::nanoseconds::rep behind a lock if it's not always lock free on all platforms. Some platforms just don't have libatomic, so this patch might cause other issues.

Wouldn't such a platform get a fatal error in llvm/cmake/modules/CheckAtomic.cmake?

Mon, Aug 10, 3:59 PM · Restricted Project
jfb added a comment to D85691: lld: link libatomic if needed for Timer.

You're probably better off putting the std::chrono::nanoseconds::rep behind a lock if it's not always lock free on all platforms. Some platforms just don't have libatomic, so this patch might cause other issues.

Mon, Aug 10, 3:31 PM · Restricted Project
jfb added a comment to D82892: [NFC] Added comparision for all types in haveSameSpecialState() of Instruction.cpp.

I didn't check that all of these compare all the required state for all opcodes.

Mon, Aug 10, 3:00 PM · Restricted Project

Wed, Aug 5

jfb added a comment to D79279: Add overloaded versions of builtin mem* functions.

Two observations that are new to me in this review:

  1. We already treat all builtins as being overloaded on address space.
  2. The revised patch treats __builtin_mem*_overloaded as being overloaded *only* on address space, volatility, and atomicity. (We've tuned the design to a point where the other qualifiers don't matter any more.)

So, we're adding three features here: overloading on (a) address space, (b) volatility, and (c) atomicity. (a) is already available in the non-_overloaded form, and we seem to be approaching agreement that (b) should be available in the non-_overloaded form too. So that only leaves (c), which is really not _overloaded but _atomic.

Based on those observations I'm now thinking that we might prefer a somewhat different approach (but one that should require only minimal changes to the patch in front of us). Specifically:

  1. Stop treating lib builtins (eg, plain memcpy) as overloaded on address space. That's a (pre-existing) conformance bug, at least for the Embedded C TR.
  2. Keep __builtin_ forms of lib builtins overloaded on address space. (No change.)
  3. Also overload __builtin_ forms of lib builtins on volatility where it makes sense, instead of adding new builtin names __builtin_mem*_overloaded.
  4. Add a new name for the builtin for the atomic forms of memcpy and memset (__builtin_memcpy_unordered_atomic maybe?).
  5. Convert the "trivial types" check from an error to a warning and apply it to all the mem* overloads. (Though I think we might actually already have such a check, so this might only require extending it to cover the atomic builtin.)

What do you think?

Wed, Aug 5, 3:02 PM · Restricted Project, Restricted Project
jfb updated the diff for D79279: Add overloaded versions of builtin mem* functions.

Remove restrict, update docs, call isCompleteType

Wed, Aug 5, 3:02 PM · Restricted Project, Restricted Project
jfb added inline comments to D79279: Add overloaded versions of builtin mem* functions.
Wed, Aug 5, 2:42 PM · Restricted Project, Restricted Project
jfb updated the diff for D79279: Add overloaded versions of builtin mem* functions.

Use 'access size' instead of 'element size'.

Wed, Aug 5, 2:42 PM · Restricted Project, Restricted Project
jfb added inline comments to D79279: Add overloaded versions of builtin mem* functions.
Wed, Aug 5, 2:37 PM · Restricted Project, Restricted Project
jfb updated the diff for D79279: Add overloaded versions of builtin mem* functions.

Add loop test requested by Vedant

Wed, Aug 5, 2:36 PM · Restricted Project, Restricted Project
jfb updated the diff for D79279: Add overloaded versions of builtin mem* functions.

Do UBSan change suggested by Vedant.

Wed, Aug 5, 2:02 PM · Restricted Project, Restricted Project
jfb added inline comments to D84947: Add libFuzzer shared object build output.
Wed, Aug 5, 11:27 AM · Restricted Project, Restricted Project

Tue, Aug 4

jfb added a comment to D79279: Add overloaded versions of builtin mem* functions.

Patch looks basically okay to me, although I'll second Richard's concern that we shouldn't absent-mindedly start producing overloaded memcpys for ordinary __builtin_memcpy.

Tue, Aug 4, 9:21 PM · Restricted Project, Restricted Project
jfb updated the diff for D79279: Add overloaded versions of builtin mem* functions.

Update docs

Tue, Aug 4, 9:21 PM · Restricted Project, Restricted Project
jfb added inline comments to D79279: Add overloaded versions of builtin mem* functions.
Tue, Aug 4, 5:57 PM · Restricted Project, Restricted Project
jfb added a comment to D79279: Add overloaded versions of builtin mem* functions.

Thanks for the detailed comments, I think I've addressed all of them! I also added UBSan support to check the builtin invocation. I think this patch is pretty much ready to go. A follow-up will need to add the support functions to compiler-rt (they're currently optional, as per https://reviews.llvm.org/D33240), and in cases where size is known we can inline them (as we do for memcpy and friends).

Tue, Aug 4, 5:51 PM · Restricted Project, Restricted Project
jfb updated the diff for D79279: Add overloaded versions of builtin mem* functions.

Address Richard's comments, add UBSan support.

Tue, Aug 4, 5:49 PM · Restricted Project, Restricted Project
jfb accepted D85239: [DOCS] Add more detail to stack protector documentation.
Tue, Aug 4, 1:33 PM · Restricted Project
jfb committed rGe18c6ef6b41a: [clang] improve diagnostics for misaligned and large atomics (authored by tschuett).
[clang] improve diagnostics for misaligned and large atomics
Tue, Aug 4, 11:11 AM
jfb closed D85102: [clang] improve diagnostics for misaligned and large atomics.
Tue, Aug 4, 11:10 AM · Restricted Project

Mon, Aug 3

jfb updated subscribers of D85044: Add __atomic_is_lock_free to compiler-rt.

This is technically a behavior change, someone like @ldionne ought to chime in, and we probably want to synchronize with libstdc++ @jwakely.

Mon, Aug 3, 9:18 PM · Restricted Project
jfb accepted D85102: [clang] improve diagnostics for misaligned and large atomics.
Mon, Aug 3, 8:39 AM · Restricted Project

Fri, Jul 31

jfb added a comment to D79279: Add overloaded versions of builtin mem* functions.

This is almost ready I think!
There are a few things still open, I'd love feedback on them.

Fri, Jul 31, 11:30 AM · Restricted Project, Restricted Project
jfb updated the diff for D79279: Add overloaded versions of builtin mem* functions.

Address Richard's comments.

Fri, Jul 31, 11:30 AM · Restricted Project, Restricted Project
jfb added a comment to D85009: [Sema][BFloat] Forbid arithmetic on vectors of bfloat..
In D85009#2187621, @jfb wrote:
In D85009#2187549, @jfb wrote:

Is that true of all vector bfloat implementations? It seems like arithmetic on these types is something implementations would likely support.

As I understand it, Arm currently has the only implementation in clang so far. But if other targets disagree, we can make this conditional on getVectorKind(), so that VectorType::NeonVector gets this restriction and other vector types get whatever they need.

You mean: only aarch64 backend supports lowering bfloat16 vectors at the moment? Because the clang support isn't "ARM bfloat", it's just bfloat. The tests are ARM bfloat and I think that's fine (i.e. Sema should be able to check ISA-specific problems), but in general this property your checking for seems like a target property.

If I write C or C++ code using bfloat, I'd like to know what that type actually means and what I can do with it. As a developer, it'll be super frustrating once other targets support bfloat... should those target have their own bfloat (because it won't be compatible with ARM's), or should bfloat work differently on different targets?

I actually don't know what the intended approach is here, which is why I'm asking :)

Yes there is an Intel bfloat type too, however we are the only target for the bfloat c/ir type so far. The jury is also out as far as the standards are concerned too, the best we can do now is prevent behavior we know is not compatible, and like Simon says, add some predication later

Fri, Jul 31, 9:25 AM · Restricted Project
jfb added a comment to D85009: [Sema][BFloat] Forbid arithmetic on vectors of bfloat..
In D85009#2187549, @jfb wrote:

Is that true of all vector bfloat implementations? It seems like arithmetic on these types is something implementations would likely support.

As I understand it, Arm currently has the only implementation in clang so far. But if other targets disagree, we can make this conditional on getVectorKind(), so that VectorType::NeonVector gets this restriction and other vector types get whatever they need.

Fri, Jul 31, 9:14 AM · Restricted Project
jfb added a comment to D85009: [Sema][BFloat] Forbid arithmetic on vectors of bfloat..

Is that true of all vector bfloat implementations? It seems like arithmetic on these types is something implementations would likely support.

Fri, Jul 31, 9:02 AM · Restricted Project

Wed, Jul 29

jfb updated subscribers of D84049: Disable use of _ExtInt with '__atomic' builtins.
In D84049#2182011, @jfb wrote:

Hi, @jfb.

_Atomic appears to already have special logic for _ExtInt in clang::Sema::BuildAtomicType. If _Atomic is used with an _ExtInt whose size is not a power of 2, or whose size is less then 8 bits, then it's disallowed. Those cases already have tests in clang/test/SemaCXX/ext-int.cpp.

Interesting. It seems like __atomic ought to be consistent with this, and we'd make it official ABI?

std::atomic appears to use and call the __atomic builtins, so it emits this patch's new diagnostic that disallows _ExtInt. Unfortunately I can't add the std::atomic<_ExtInt(N)> to a sema lit test because I can't #include the real atomic, but I did verify it locally.

Can you add the test to libc++? @ldionne WDYT?

IF we are going as far as modifying libc++, perhaps we can just add a SFINAE test to std::atomic to disallow the _ExtInt types? That said, it won't help with libstdc++.

Either way, I suspect that is better placed in a different patch, and should no longer hold up this patch.

Wed, Jul 29, 9:28 AM
jfb updated subscribers of D84049: Disable use of _ExtInt with '__atomic' builtins.

Hi, @jfb.

_Atomic appears to already have special logic for _ExtInt in clang::Sema::BuildAtomicType. If _Atomic is used with an _ExtInt whose size is not a power of 2, or whose size is less then 8 bits, then it's disallowed. Those cases already have tests in clang/test/SemaCXX/ext-int.cpp.

Wed, Jul 29, 8:23 AM

Tue, Jul 28

jfb committed rG389f009c5757: [NFC] Sema: use checkArgCount instead of custom checking (authored by jfb).
[NFC] Sema: use checkArgCount instead of custom checking
Tue, Jul 28, 1:42 PM
jfb closed D84666: [NFC] Sema: use checkArgCount instead of custom checking.
Tue, Jul 28, 1:41 PM · Restricted Project

Mon, Jul 27

jfb added inline comments to D79279: Add overloaded versions of builtin mem* functions.
Mon, Jul 27, 5:29 PM · Restricted Project, Restricted Project
jfb updated subscribers of D79279: Add overloaded versions of builtin mem* functions.

I've addressed @rsmith @rjmccall suggestions (unless noted), thanks!
An open question: as of 6e4aa1e48138182685431c76184dfc36e620aea2 @dneilson added an assertion on CreateElementUnorderedAtomicMemCpy to check that the pointer arguments have alignments of at least the element size. That makes sense when the IR is only ever built internally to LLVM, but now that I'm adding a builtin it's more of a dynamic property. Should I also force this in the frontend (understanding that alignment isn't always well known at compile time), or should simply assume that the alignment is correct because it's a dynamic property?

Mon, Jul 27, 5:24 PM · Restricted Project, Restricted Project
jfb updated the diff for D79279: Add overloaded versions of builtin mem* functions.

Address comments

Mon, Jul 27, 5:24 PM · Restricted Project, Restricted Project
Herald added a project to D84666: [NFC] Sema: use checkArgCount instead of custom checking: Restricted Project.
Mon, Jul 27, 8:34 AM · Restricted Project

Thu, Jul 23

jfb added a comment to D79279: Add overloaded versions of builtin mem* functions.

Do you think it'd be useful to have different guarantees for different operands? I guess it could come up, but it'd be a whole lot of extra complexity that I can't imagine we'd ever support.

You mean, if element_size is passed then you get different guarantees?

No, sorry, I mean different guarantees for the different pointer operands. In principle, we could allow you to say that the memcpy has to be done with 4-byte accesses from the source and 2-byte accesses to the destination. That's implementable but a lot of work.

Thu, Jul 23, 11:30 AM · Restricted Project, Restricted Project
jfb added a comment to D79279: Add overloaded versions of builtin mem* functions.

I think the argument is treated as if it were 1 if not given. That's all that ordinary memcpy formally guarantees, which seems to work fine (semantically, if not performance-wise) for pretty much everything today.

Thu, Jul 23, 10:52 AM · Restricted Project, Restricted Project
jfb added a comment to D79279: Add overloaded versions of builtin mem* functions.

I don't think any of these should allow _Atomic unless we're going to give it some sort of consistent atomic semantics (which is hard to imagine being useful), and I think you should just take an extra argument of the minimum access width on all of them uniformly if you think that's important. Builtins can have optional arguments.

Thu, Jul 23, 10:29 AM · Restricted Project, Restricted Project
jfb added a comment to D79279: Add overloaded versions of builtin mem* functions.

My point is that this has nothing to do with the ordinary semantics of _Atomic. You're basically just looking at the word "atomic" and saying that, hey, a minimum access size is sortof related to atomicity.

If you want this to be able to control the minimum access size, you should allow that to be passed in as an optional argument instead.

Thu, Jul 23, 10:17 AM · Restricted Project, Restricted Project
jfb updated the diff for D79279: Add overloaded versions of builtin mem* functions.

Re-update

Thu, Jul 23, 8:21 AM · Restricted Project, Restricted Project
jfb updated the diff for D79279: Add overloaded versions of builtin mem* functions.

Improve documentation

Thu, Jul 23, 8:20 AM · Restricted Project, Restricted Project
jfb added a comment to D79279: Add overloaded versions of builtin mem* functions.
In D79279#2168533, @jfb wrote:

Is there a need for an atomic memcpy at all? Why is it useful to allow this operation to take on "atomic" semantics — which aren't actually atomic because the loads and stores to elements are torn — with hardcoded memory ordering and somewhat arbitrary rules about what the atomic size is?

Hans lays out a rationale for usefulness in his paper, but what I've implemented is more useful: it's *unordered* so you can fence as you desire around it, yet it guarantees a minimum memory access size based on the pointer parameters. For example, copying an atomic int will be 4 byte operations which are single-copy-atomic, but the accesses from one int to the next aren't performed in any guaranteed order (or observable in any guaranteed order either). I talked about this with him a while ago but IIRC he wasn't sure about implementation among other things, so when you asked me to widen my original volatile-only memcpy to also do other qualifiers, I realized that it was a neat way to do atomic as well as other qualifiers. I've talked to a few SG1 folks about this, and I believe (for other reasons too) it's where the design will end up for Hans' paper.

I can see the usefulness of this operation, but it seems like a odd semantic mismatch for what is basically just a memcpy where one of the pointers happens to have _Atomic type, like you're shoe-horning it into this builtin just to avoid declaring a different one.

Thu, Jul 23, 8:09 AM · Restricted Project, Restricted Project

Wed, Jul 22

jfb added a comment to D79279: Add overloaded versions of builtin mem* functions.

Is there a need for an atomic memcpy at all? Why is it useful to allow this operation to take on "atomic" semantics — which aren't actually atomic because the loads and stores to elements are torn — with hardcoded memory ordering and somewhat arbitrary rules about what the atomic size is?

Wed, Jul 22, 8:32 PM · Restricted Project, Restricted Project
jfb added inline comments to D79279: Add overloaded versions of builtin mem* functions.
Wed, Jul 22, 5:39 PM · Restricted Project, Restricted Project
jfb updated the diff for D79279: Add overloaded versions of builtin mem* functions.

Address all but one of John's comments

Wed, Jul 22, 5:39 PM · Restricted Project, Restricted Project
jfb updated the diff for D79279: Add overloaded versions of builtin mem* functions.

Follow John's suggestions

Wed, Jul 22, 11:26 AM · Restricted Project, Restricted Project

Tue, Jul 21

jfb added a comment to D71726: Let clang atomic builtins fetch add/sub support floating point types.

Why not have clang always emit atomicrmw for floats, and let AtomicExpandPass handle legalizing that into integer atomics if necessary, rather than adding a target hook in clang?

Not all targets can legalize fp atomics by AtomicExpandPass. Some targets need library support.

Tue, Jul 21, 3:22 PM

Mon, Jul 20

jfb added a comment to D84049: Disable use of _ExtInt with '__atomic' builtins.

Might be worth thinking about what volatile _ExtInt(1) ought to do as well :)

Mon, Jul 20, 9:16 PM
jfb added a comment to D84049: Disable use of _ExtInt with '__atomic' builtins.

I think you also want tests with the following:

#include <atomic>
Mon, Jul 20, 9:16 PM

Mon, Jul 13

jfb added a comment to D83340: Prohibit use of _ExtInt in atomic intrinsic.

@jfb The __atomic builtins seem to round up to the nearest power of 2, and they appear to work because of that. Is there a specific use case or reproducer you were concerned about?

Mon, Jul 13, 7:56 PM · Restricted Project

Jul 10 2020

jfb committed rG7bf73bcf6d93: [docs] LLVM Security Group and Process (authored by jfb).
[docs] LLVM Security Group and Process
Jul 10 2020, 3:28 PM
jfb closed D70326: [docs] LLVM Security Group and Process.
Jul 10 2020, 3:28 PM · Restricted Project
jfb added inline comments to D70326: [docs] LLVM Security Group and Process.
Jul 10 2020, 1:57 PM · Restricted Project
jfb updated subscribers of D70326: [docs] LLVM Security Group and Process.
Jul 10 2020, 11:28 AM · Restricted Project
jfb added a comment to D70326: [docs] LLVM Security Group and Process.

I believe this is now ready to go, with more to do afterwards.

Jul 10 2020, 11:24 AM · Restricted Project
jfb added inline comments to D70326: [docs] LLVM Security Group and Process.
Jul 10 2020, 11:24 AM · Restricted Project
jfb updated the diff for D70326: [docs] LLVM Security Group and Process.

Address more comments, add names.

Jul 10 2020, 11:24 AM · Restricted Project
jfb updated subscribers of D83361: [LLVM] Add libatomic load/store functions to TargetLibraryInfo.

@dschuff can help answer the WebAssembly question.

Jul 10 2020, 9:58 AM · Restricted Project

Jul 9 2020

jfb added a comment to D83509: <rdar://problem/63335596> CrashTracer: clang at clang: llvm::BitstreamWriter::ExitBlock.

I don't remember if this will auto-close, since I forgot to add the Phabricator ID to the commit message. In any case it's in: 00c9a504aeed2603bd8bc9b89d753534e929c8e8

Jul 9 2020, 8:33 PM · Restricted Project
jfb committed rG00c9a504aeed: CrashTracer: clang at clang: llvm::BitstreamWriter::ExitBlock (authored by ojhunt).
CrashTracer: clang at clang: llvm::BitstreamWriter::ExitBlock
Jul 9 2020, 8:29 PM
jfb accepted D83509: <rdar://problem/63335596> CrashTracer: clang at clang: llvm::BitstreamWriter::ExitBlock.
Jul 9 2020, 2:26 PM · Restricted Project
jfb added a comment to D75229: [clang-tidy] Add signal-in-multithreaded-program check.

It seems like you don't want this check to trigger on POSIX platforms, given:

Exceptions CON37-C-EX1: Implementations such as POSIX that provide defined behavior when multithreaded programs use custom signal handlers are exempt from this rule [IEEE Std 1003.1-2013].

Jul 9 2020, 8:36 AM · Restricted Project, Restricted Project
jfb added inline comments to D77493: [clang-tidy] Add do-not-refer-atomic-twice check.
Jul 9 2020, 8:31 AM · Restricted Project, Restricted Project
jfb added inline comments to D83465: Encode alignment attribute for `atomicrmw`.
Jul 9 2020, 8:23 AM · Restricted Project
jfb accepted D83375: [NFC] Separate bitcode reading for FUNC_CODE_INST_CMPXCHG(_OLD).
Jul 9 2020, 8:15 AM · Restricted Project
jfb added inline comments to D83375: [NFC] Separate bitcode reading for FUNC_CODE_INST_CMPXCHG(_OLD).
Jul 9 2020, 8:15 AM · Restricted Project

Jul 7 2020

jfb added a comment to D83340: Prohibit use of _ExtInt in atomic intrinsic.

Are the _Extint uses here as you'd expect too? (using _Atomic): https://reviews.llvm.org/D79279
We have other atomic primitives, such as _atomic_, do these need updates too?

Jul 7 2020, 1:12 PM · Restricted Project

Jul 6 2020

jfb added inline comments to D83136: [NFC] Adding the align attribute on Atomic{CmpXchg|RMW}Inst.
Jul 6 2020, 2:48 PM · Restricted Project

Jul 3 2020

jfb added inline comments to D83136: [NFC] Adding the align attribute on Atomic{CmpXchg|RMW}Inst.
Jul 3 2020, 11:52 AM · Restricted Project

Jul 2 2020

jfb added inline comments to D79279: Add overloaded versions of builtin mem* functions.
Jul 2 2020, 5:50 PM · Restricted Project, Restricted Project
jfb updated the diff for D79279: Add overloaded versions of builtin mem* functions.

Address comments.

Jul 2 2020, 5:50 PM · Restricted Project, Restricted Project

Jul 1 2020

jfb updated the diff for D79279: Add overloaded versions of builtin mem* functions.

Add memmove and memset (but still missing the codegen tests)

Jul 1 2020, 10:43 PM · Restricted Project, Restricted Project
jfb updated the diff for D79279: Add overloaded versions of builtin mem* functions.

Arcanist ate the rest of my commit and I am confused.

Jul 1 2020, 4:13 PM · Restricted Project, Restricted Project
jfb updated the diff for D79279: Add overloaded versions of builtin mem* functions.

This builtin doesn't return anything.

Jul 1 2020, 4:13 PM · Restricted Project, Restricted Project
jfb updated the summary of D79279: Add overloaded versions of builtin mem* functions.
Jul 1 2020, 9:42 AM · Restricted Project, Restricted Project
jfb updated the diff for D79279: Add overloaded versions of builtin mem* functions.

Overload a new builtin instead. For now I only did memcpy, to get feedback on the approach. I'll add other mem* functions if this makes sense.

Jul 1 2020, 9:42 AM · Restricted Project, Restricted Project
jfb updated the summary of D79279: Add overloaded versions of builtin mem* functions.
Jul 1 2020, 9:42 AM · Restricted Project, Restricted Project
jfb retitled D79279: Add overloaded versions of builtin mem* functions from Allow volatile parameters to __builtin_mem{cpy,move,set} to Add overloaded versions of builtin mem* functions.
Jul 1 2020, 9:42 AM · Restricted Project, Restricted Project

Jun 30 2020

jfb committed rGca134e4c525b: [NFC] fix diagnostic (authored by jfb).
[NFC] fix diagnostic
Jun 30 2020, 10:11 PM
jfb committed rGc7586444ca78: Fix diagnostic for missing virtual dtor (authored by jfb).
Fix diagnostic for missing virtual dtor
Jun 30 2020, 9:39 PM
jfb added inline comments to D82673: [ModuloSchedule] Make PeelingModuloScheduleExpander inheritable..
Jun 30 2020, 9:39 PM · Restricted Project
jfb added a comment to D82730: [SimplifyCFG] Merge identical basic blocks (WIP).

I like this, but it's going to run into the same issues as MergeFuncs. Can you sync with @hiraditya and @vish99 to solve both block comparison and function comparison using the same methodology? The we can turn both of them on by default. Or in other words, how does this not have exactly the same issues MergeFuncs has? The principal one is that the IR comparator goes out of sync with IR definition, and we get miscompiles because of this.

Jun 30 2020, 10:50 AM · Restricted Project
jfb updated subscribers of D82730: [SimplifyCFG] Merge identical basic blocks (WIP).
Jun 30 2020, 10:50 AM · Restricted Project
jfb accepted D82832: Correctly generate invert xor value for Binary Atomics of int size > 64.

This amused me.

Jun 30 2020, 9:44 AM · Restricted Project

Jun 29 2020

jfb updated subscribers of D81369: [Alignment][NFC] Migrate AtomicExpandPass to Align.

Adding aliment to these operations will also require updating MergeFuncs /cc @vish99 @hiraditya

Jun 29 2020, 10:48 AM · Restricted Project

Jun 26 2020

jfb committed rGb10bd6dfc621: [NFC] Bump ObjCOrBuiltinIDBits to 15 (authored by jfb).
[NFC] Bump ObjCOrBuiltinIDBits to 15
Jun 26 2020, 2:09 PM
jfb committed rG13fdcd37b325: [NFC] Builtins: list 'R' for restrict (authored by jfb).
[NFC] Builtins: list 'R' for restrict
Jun 26 2020, 1:09 PM

Jun 23 2020

jfb added inline comments to D81765: Don't inline dynamic allocas that simplify to huge static allocas..
Jun 23 2020, 9:03 PM · Restricted Project
jfb added a comment to D81765: Don't inline dynamic allocas that simplify to huge static allocas..
In D81765#2090952, @jfb wrote:

One thing I'd like to make sure we don't break:

__attribute__((always_inline))
char *stack_allocate(size_t size) {
  if (size < threshold)
    return alloca(size);
  return malloc(size);
}

This should always inline. Is it still the case? It turns out that we have Important Code which relies on this...

Jun 23 2020, 11:48 AM · Restricted Project

Jun 19 2020

jfb abandoned D80055: Diagnose union tail padding.

I've gotten what I wanted out of this (diagnosed a particular codebase), and am not sure it's worth pursuing further. If anyone is interested, please let me know.

Jun 19 2020, 3:14 PM · Restricted Project

Jun 17 2020

jfb added a comment to D79249: [NOT FOR REVIEW] Experimental support for zero-or-trap behavior for uninitialized variables..

Overall I like this approach.

Jun 17 2020, 5:17 PM · Restricted Project, Restricted Project
jfb added inline comments to D81790: Added hasSamePropertiesAs method for CmpXchgInst and FenceInst.
Jun 17 2020, 10:13 AM · Restricted Project
jfb updated subscribers of D81348: [compiler-rt][builtins] Add tests for atomic builtins support functions.
Jun 17 2020, 9:40 AM · Restricted Project

Jun 15 2020

jfb added inline comments to D81348: [compiler-rt][builtins] Add tests for atomic builtins support functions.
Jun 15 2020, 10:23 AM · Restricted Project

Jun 12 2020

jfb added a comment to D81765: Don't inline dynamic allocas that simplify to huge static allocas..

One thing I'd like to make sure we don't break:

__attribute__((always_inline))
char *stack_allocate(size_t size) {
  if (size < threshold)
    return alloca(size);
  return malloc(size);
}

This should always inline. Is it still the case? It turns out that we have Important Code which relies on this...

Jun 12 2020, 3:58 PM · Restricted Project
jfb updated subscribers of D81765: Don't inline dynamic allocas that simplify to huge static allocas..
Jun 12 2020, 3:24 PM · Restricted Project
jfb added inline comments to D81733: GlobalISel: Don't fail translate on weak cmpxchg.
Jun 12 2020, 9:15 AM · Restricted Project
jfb added inline comments to D81733: GlobalISel: Don't fail translate on weak cmpxchg.
Jun 12 2020, 8:38 AM · Restricted Project

Jun 11 2020

jfb added a comment to D81602: [libc++] Provide SEM_VALUE_MAX fallback on Solaris.
In D81602#2087793, @ro wrote:

Let's put it that way: as far as I can tell, this code is dead, so this patch isn't adding anything beyond technical debt. And the reason why it's dead is that there are no *libc++* build bots set up for Solaris, and no maintainer actively working on supporting that platform AFAICT.

True, because right now libc++ doesn't build even on Illumos. As I mentioned, my patch is only the first in a series to change that.
I'd have posted more of them already but for the pushback I got on the first trivial one. While working on my patch series, I already encountered more of those (like AIX and partially Windows) that caused me to spend quite some time trying to verify that my changes wouldn't negatively affect them, only to discover in the end that the code doesn't even compile.

JF made the comment he made because I recently complained to him about how the maintenance burden for libc++ is both huge and also rests on the shoulder of too few. The main reason for the burden being so large is that we try to support a high number of system configurations and sub-configurations of libc++ (like no-threads, no-stdio, etc.), and we also tend to bend backwards instead of requesting that systems conform to e.g. POSIX in a more stringent way. That's unfortunately part of a discussion that needs to happen and that is much larger than this patch, but my point is that libc++ needs to start dropping workarounds for systems that are not actively supported, not add new ones.

Fully understood: keeping code around for dead systems is only creating work for everyone else and making the code harder to maintain
for no gain. However, demanding systems that conform to the very latest C and POSIX.1 standards quickly restricts you to a very small set: many people tend to forget that.

Jun 11 2020, 3:26 PM · Restricted Project
jfb added a comment to D70326: [docs] LLVM Security Group and Process.

This has received good feedback, I'll therefore commit it in the next few days.

Jun 11 2020, 8:46 AM · Restricted Project