jfb (JF Bastien)
User

Projects

User does not belong to any projects.

User Details

User Since
Jan 27 2014, 9:36 AM (233 w, 3 d)

Recent Activity

Today

jfb updated subscribers of D47618: __c11_atomic_load's _Atomic can be const.
Thu, Jul 19, 2:57 PM
jfb added a reviewer for D47618: __c11_atomic_load's _Atomic can be const: Anastasia.
Thu, Jul 19, 2:45 PM

Yesterday

jfb committed rC337410: Support implicit _Atomic struct load / store.
Support implicit _Atomic struct load / store
Wed, Jul 18, 11:07 AM
jfb committed rL337410: Support implicit _Atomic struct load / store.
Support implicit _Atomic struct load / store
Wed, Jul 18, 11:06 AM
jfb closed D49458: Support implicit _Atomic struct load / store.
Wed, Jul 18, 11:06 AM
jfb added a comment to D49458: Support implicit _Atomic struct load / store.

I'm sure there are other bugs around _Atomic, please file a bug an CC me if that's the case. I'll commit this fix for now.

Wed, Jul 18, 11:05 AM

Tue, Jul 17

jfb added a comment to D46112: Allow _Atomic to be specified on incomplete types.

Also needs some test coverage for atomic operations which aren't calls, like "typedef struct S S; void f(_Atomic S *s, _Atomic S *s2) { *s = *s2; };".

Thank you for pointing this out -- that uncovered an issue where we were not properly diagnosing the types as being incomplete. I've added a new test case and rolled the contents of Sema\atomic-type.cpp (which I added in an earlier patch) into SemaCXX\atomic-type.cpp (which already existed and I missed it).

I don't think this has sufficiently addressed the comment. Specifically, for a case like:

struct NotTriviallyCopyable { NotTriviallyCopyable(const NotTriviallyCopyable&); };
void f(_Atomic NotTriviallyCopyable *p) { *p = *p; }

... we should reject the assignment, because it would perform an atomic operation on a non-trivially-copyable type. It would probably suffice to check the places where we form an AtomicToNonAtomic or NonAtomicToAtomic conversion.

In passing, I noticed that this crashes Clang (because we fail to create an lvalue-to-rvalue conversion for x):

struct X {};                              
void f(_Atomic X *p, X x) { *p = x; }

This will likely get in the way of your test cases unless you fix it :)

It only gets in the way for C++ whereas my primary concern for this patch is C. I tried spending a few hours on this today and got nowhere -- we have a lot of FIXME comments surrounding atomic type qualifiers in C++. I've run out of time to be able to work on this patch and may have to abandon it. I'd hate to lose the forward progress already made, so I'm wondering if the C bits are sufficiently baked that even more FIXMEs around atomics in C++ would suffice?

Tue, Jul 17, 4:46 PM
jfb created D49458: Support implicit _Atomic struct load / store.
Tue, Jul 17, 4:45 PM

Fri, Jul 13

jfb committed rC337041: CodeGen: specify alignment + inbounds for automatic variable initialization.
CodeGen: specify alignment + inbounds for automatic variable initialization
Fri, Jul 13, 1:38 PM
jfb committed rL337041: CodeGen: specify alignment + inbounds for automatic variable initialization.
CodeGen: specify alignment + inbounds for automatic variable initialization
Fri, Jul 13, 1:38 PM
jfb closed D49209: CodeGen: specify alignment for automatic variable initialization.
Fri, Jul 13, 1:38 PM
jfb added inline comments to D49209: CodeGen: specify alignment for automatic variable initialization.
Fri, Jul 13, 10:26 AM
jfb updated the diff for D49209: CodeGen: specify alignment for automatic variable initialization.
  • Simplify CreateStore.
Fri, Jul 13, 10:22 AM
jfb added a comment to D49194: [WebAssembly] Add tests for weaker memory consistency orderings.

Currently LLVM treats all atomic IR instructions as volatile. Link0 Link1 So I'm not exactly sure what you mean by 'upgrade volatile to seq_cst' is. For non-atomic volatile memory instructions, they are not atomic, so they don't have orderings.

Fri, Jul 13, 10:05 AM

Thu, Jul 12

jfb updated the diff for D49209: CodeGen: specify alignment for automatic variable initialization.
  • Fix silly naming and lookup.
Thu, Jul 12, 3:34 PM
jfb added a comment to D49209: CodeGen: specify alignment for automatic variable initialization.

I updated the patch to use Address, and also use inbounds. This was a bit tricky because of the GEPs. I also updated tests which run into this codegen. Only the following tests actually hit this code, and not all check these stores:

Thu, Jul 12, 3:26 PM
jfb updated the diff for D49209: CodeGen: specify alignment for automatic variable initialization.
  • Use Address as suggested in review.
Thu, Jul 12, 3:25 PM
jfb added a comment to D49194: [WebAssembly] Add tests for weaker memory consistency orderings.

@jfb just as a sanity check, it should always be valid to just "upgrade" LLVM's orderings and select them all as the sequentially consistent wasm ops, right?

Thu, Jul 12, 12:29 PM

Wed, Jul 11

jfb created D49209: CodeGen: specify alignment for automatic variable initialization.
Wed, Jul 11, 5:16 PM
jfb committed rL336840: [NFC] typo.
[NFC] typo
Wed, Jul 11, 12:56 PM
jfb committed rC336840: [NFC] typo.
[NFC] typo
Wed, Jul 11, 12:56 PM

Tue, Jul 10

jfb committed rL336730: [NFC] typo.
[NFC] typo
Tue, Jul 10, 2:57 PM

Thu, Jul 5

jfb accepted D48852: [Sema] -Wformat-pedantic only for NSInteger/NSUInteger %tu/%td on Darwin.

LGTM after a few questions.

Thu, Jul 5, 11:00 AM

Fri, Jun 22

jfb committed rC335393: [Sema] -Wformat-pedantic only for NSInteger/NSUInteger %zu/%zi on Darwin.
[Sema] -Wformat-pedantic only for NSInteger/NSUInteger %zu/%zi on Darwin
Fri, Jun 22, 2:59 PM
jfb committed rL335393: [Sema] -Wformat-pedantic only for NSInteger/NSUInteger %zu/%zi on Darwin.
[Sema] -Wformat-pedantic only for NSInteger/NSUInteger %zu/%zi on Darwin
Fri, Jun 22, 2:59 PM
jfb closed D47290: [Sema] -Wformat-pedantic only for NSInteger/NSUInteger %zu/%zi on Darwin.
Fri, Jun 22, 2:59 PM

Jun 8 2018

jfb added a comment to D47290: [Sema] -Wformat-pedantic only for NSInteger/NSUInteger %zu/%zi on Darwin.

Okay, that's fair, but the vendor-specific type for my Windows example is spelled DWORD. I'm really worried that this special case will become a precedent and we'll wind up with -Wformat being relaxed for everything based on the same rationale. If that's how the community wants -Wformat to work, cool, but I'd like to know if we're intending to change (what I see as) the design of this warning.

I spoke with @jfb offline and am less concerned about this patch now. He's welcome to correct me if I misrepresent anything, but the precedent this sets is that a platform "owner" (someone with authority, not Joe Q Random-User) can relax -Wformat as in this patch, but this is not a precedent for a blanket change to -Wformat just because the UB happens to work and we "know" it.

Jun 8 2018, 11:18 PM

Jun 4 2018

jfb added a comment to D46024: [clang-format] Add SpaceBeforeCpp11BracedList option..

FWIW, please note that this space-before-brace style is not specific to WebKit; CppCoreGuidelines exhibits it as well:
http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#es23-prefer-the--initializer-syntax

Jun 4 2018, 11:02 AM

Jun 1 2018

jfb committed rCXX333776: Mark __c11_atomic_load as const.
Mark __c11_atomic_load as const
Jun 1 2018, 11:08 AM
jfb committed rL333776: Mark __c11_atomic_load as const.
Mark __c11_atomic_load as const
Jun 1 2018, 11:07 AM
jfb closed D47613: Mark __c11_atomic_load as const.
Jun 1 2018, 11:07 AM
jfb updated subscribers of D47589: [RISCV] Add codegen support for atomic load/stores with RV32A.
In D47589#1118174, @jfb wrote:

The compiler-rt "__atomic_*" are known to be buggy; see https://reviews.llvm.org/D45321

Seems you're advocating that this patch (and the LangRef) follow the reality you'd like to have, not the one we actually have :-)
So I'll re-iterate: is the alignment check a documented guarantee that __atomic_* functions must provide, in all of their implementations?

The compiler must not emit a call to the __atomic_load_4 (and other names with sizes on the end) functions on unaligned memory -- that function can assume its argument is aligned.

But __atomic_load (size is given as an argument) must be safe to call on both aligned and unaligned memory, *and* must safely interoperate both with __atomic_load_4 and inline-emitted atomics if the current hardware supports them.

Jun 1 2018, 9:49 AM

May 31 2018

jfb committed rCXX333723: Filesystem tests: un-confuse write time.
Filesystem tests: un-confuse write time
May 31 2018, 10:04 PM
jfb committed rL333723: Filesystem tests: un-confuse write time.
Filesystem tests: un-confuse write time
May 31 2018, 10:04 PM
jfb closed D47557: Filesystem tests: un-confuse write time.
May 31 2018, 10:03 PM
jfb updated the diff for D47557: Filesystem tests: un-confuse write time.
  • Add back directory test
May 31 2018, 10:02 PM
jfb added a comment to D47618: __c11_atomic_load's _Atomic can be const.

Note that I don't touch the OpenCL __constant stuff, because the separate address space means this can be actually read-only, which means you can't cmpxchg for very wide reads.

May 31 2018, 9:57 PM
jfb updated the summary of D47613: Mark __c11_atomic_load as const.
May 31 2018, 9:56 PM
jfb created D47618: __c11_atomic_load's _Atomic can be const.
May 31 2018, 9:55 PM
jfb created D47613: Mark __c11_atomic_load as const.
May 31 2018, 4:39 PM
jfb updated the diff for D47557: Filesystem tests: un-confuse write time.
  • Remove access time checks, simplify existing check, after talking to EricWF on IRC.
May 31 2018, 3:53 PM
jfb added a comment to D47589: [RISCV] Add codegen support for atomic load/stores with RV32A.

Seems you're advocating that this patch (and the LangRef) follow the reality you'd like to have, not the one we actually have :-)

My description matches the way GNU libatomic works in practice, as far as I know. (compiler-rt's implementation is incomplete and broken in other ways anyway; see https://reviews.llvm.org/D47606.)

May 31 2018, 2:49 PM
jfb accepted D47606: [compiler-rt] [builtins] Don't build __atomic_load etc. by default..

Wow this is amazing!

May 31 2018, 2:04 PM
jfb added a comment to D47589: [RISCV] Add codegen support for atomic load/stores with RV32A.

The compiler-rt "__atomic_*" are known to be buggy; see https://reviews.llvm.org/D45321

May 31 2018, 2:03 PM
jfb added a comment to D47589: [RISCV] Add codegen support for atomic load/stores with RV32A.

Say I have two TUs which share an atomic location, but have a different idea of its alignment. They both access that location, one with libcall and one with an instruction. That's clearly broken. Can this happen?

No, it's not broken (assuming the location is actually aligned at runtime). In the translation unit where the location is known aligned, it'll use the native lock-free instruction. In the other translation unit, the libatomic call will check the alignment of the address at runtime, see it's aligned, and use a compatible lock-free implementation.

May 31 2018, 1:10 PM
jfb added a comment to D47589: [RISCV] Add codegen support for atomic load/stores with RV32A.

Making it the behavior we choose is fine with me. I just understood your comment to mean that you wanted to give this semantics, and that's more work than just stating the semantics you want.

May 31 2018, 12:46 PM
jfb added a comment to D47587: [RISCV] Codegen support for atomic operations on RV32I.

Looks fine in general, though I know ~nothing of RISCV.

May 31 2018, 12:42 PM
jfb added a reviewer for D47557: Filesystem tests: un-confuse write time: compnerd.
May 31 2018, 9:17 AM
jfb added a comment to D47553: Add TargetLowering::shouldExpandAtomicToLibCall and query it from AtomicExpandPass.
In D47553#1117633, @asb wrote:

Updated patch to document that shouldExpandAtomicToLibCall should return the same result for objects of the same size.

@jfb: I'm simply replicating the existing logic with regards to checking alignment - no functional change is intended.

May 31 2018, 9:07 AM
jfb added a comment to D47553: Add TargetLowering::shouldExpandAtomicToLibCall and query it from AtomicExpandPass.
In D47553#1117763, @asb wrote:

On other architectures which have 32-bit atomics, but not 8 and 16-bit atomics, it's generally possible to lower smaller operations using a 32-bit cmpxchg loop. Does that not work on RISCV for some reason?

We can absolutely lower to ll+sc, which is what the 8 and 16-bit libatomic implementations do for rv32ia. My thought was simply:

  1. Simply generating the libcall is easy and correct. This gives a stepping stone allowing the straight-forward implementation of initial support that can then be improved by switching to generating in-line instruction sequences
  2. If we are happy that the 8 and 16bit __atomic_* libcalls are lock-free, it is safe to choose between inline instruction sequences and libcalls freely. In that case, a hook such as this would allow you to use the libcall when optimising for code size.

    I could understand an argument that use-case 1) isn't compelling enough to add this new hook, and determining whether use-case 2) makes sense is dependent on the conclusion of the GCC bug 86005 discussion. Thanks @jyknight for sharing your thoughts in that GCC thread - much appreciated.
May 31 2018, 9:02 AM
jfb added a comment to D47589: [RISCV] Add codegen support for atomic load/stores with RV32A.
In D47589#1117778, @jfb wrote:

When the A extension is supported, __atomic libcalls will be generated for any atomic that isn't the native word size or has less than natural alignment.

When do you expect non-natural alignment to occur? Is this purely for C++ support? If so you're guaranteed natural alignment. Otherwise (for intrinsics or for other languages) I'd like to understand what you expect, and whether you have the guarantee that the alignment information you have is correct. Without knowing that it's absolutely correct you're going to codegen bad code (a libcall in one place, and instructions in another).

May 31 2018, 8:53 AM
jfb added inline comments to D47587: [RISCV] Codegen support for atomic operations on RV32I.
May 31 2018, 8:47 AM
jfb added a comment to D47589: [RISCV] Add codegen support for atomic load/stores with RV32A.

When the A extension is supported, __atomic libcalls will be generated for any atomic that isn't the native word size or has less than natural alignment.

May 31 2018, 8:42 AM

May 30 2018

jfb updated the summary of D47557: Filesystem tests: un-confuse write time.
May 30 2018, 2:11 PM
jfb created D47557: Filesystem tests: un-confuse write time.
May 30 2018, 2:10 PM
jfb added a comment to D47553: Add TargetLowering::shouldExpandAtomicToLibCall and query it from AtomicExpandPass.

Could you detail what type of code will generate this? In C++ the standard library is responsible for guaranteeing properly sized and aligned atomics. Size will always be accurate, but I'm worried that your change wants perfect alignment information and you might not have that information. Say two pieces of code communicate through the same atomic location, but they have different alignment information, and one gets a libcall and the other doesn't... You're in trouble.

May 30 2018, 1:33 PM
jfb added inline comments to D46084: [Fixed Point Arithmetic] Addition of the Fixed Point _Accum type.
May 30 2018, 10:41 AM · Restricted Project

May 29 2018

jfb committed rL333479: Mark deduction guide tests as failing on apple-clang-9.
Mark deduction guide tests as failing on apple-clang-9
May 29 2018, 4:32 PM
jfb committed rCXX333479: Mark deduction guide tests as failing on apple-clang-9.
Mark deduction guide tests as failing on apple-clang-9
May 29 2018, 4:32 PM
jfb added a comment to D47290: [Sema] -Wformat-pedantic only for NSInteger/NSUInteger %zu/%zi on Darwin.
In D47290#1113412, @jfb wrote:

This is relaxing -Wformat and making users instead write -Wformat-pedantic to get the strong guarantees, which is the opposite of what I thought the consensus was. Have I misunderstood something?

From Shoaib's email:

Based on all the discussion so far, I think the consensus is that we don't want to relax -Wformat by default, but an additional relaxed option would be fine. That works for me (where I can just switch my codebases to use the new warning option), but it wouldn't handle JF Bastien's use case, so he would still want to pursue the relaxations specific to Apple platforms.

This patch is specific to the Darwin platform, as proposed in Alex' original patch and as I promised to do in the cfe-dev thread. Shoaib's follow-up would be different and do what I think you're referring to.

There were several people on the thread asking to not relax -Wformat for any target, but instead wanted a separate warning flag to perform a more relaxed check.

http://lists.llvm.org/pipermail/cfe-dev/2018-May/057938.html

May 29 2018, 2:49 PM

May 26 2018

jfb committed rCXX333351: Revert "Add nonnull; use it for atomics".
Revert "Add nonnull; use it for atomics"
May 26 2018, 12:50 PM
jfb committed rL333351: Revert "Add nonnull; use it for atomics".
Revert "Add nonnull; use it for atomics"
May 26 2018, 12:50 PM
jfb added a comment to D47290: [Sema] -Wformat-pedantic only for NSInteger/NSUInteger %zu/%zi on Darwin.

Addressed comments.

May 26 2018, 12:36 PM
jfb updated the diff for D47290: [Sema] -Wformat-pedantic only for NSInteger/NSUInteger %zu/%zi on Darwin.
  • Fix variable capitalization.
May 26 2018, 12:36 PM
jfb added a comment to D47290: [Sema] -Wformat-pedantic only for NSInteger/NSUInteger %zu/%zi on Darwin.

This is relaxing -Wformat and making users instead write -Wformat-pedantic to get the strong guarantees, which is the opposite of what I thought the consensus was. Have I misunderstood something?

May 26 2018, 12:20 PM

May 25 2018

jfb added 1 commit(s) for D47225: Add nonnull; use it for atomics: rCXX333327: Fix GCC handling of ATOMIC_VAR_INIT.
May 25 2018, 5:18 PM
jfb added an edge to rCXX333327: Fix GCC handling of ATOMIC_VAR_INIT: D47225: Add nonnull; use it for atomics.
May 25 2018, 5:18 PM
jfb committed rCXX333327: Fix GCC handling of ATOMIC_VAR_INIT.
Fix GCC handling of ATOMIC_VAR_INIT
May 25 2018, 5:18 PM
jfb committed rL333327: Fix GCC handling of ATOMIC_VAR_INIT.
Fix GCC handling of ATOMIC_VAR_INIT
May 25 2018, 5:18 PM
jfb added a comment to D47225: Add nonnull; use it for atomics.

GCC in libcxx-libcxxabi-x86_64-linux-ubuntu-cxx03 seems to mis-handle ATOMIC_VAR_INIT:

May 25 2018, 5:15 PM
jfb committed rCXX333325: Add nonnull; use it for atomics.
Add nonnull; use it for atomics
May 25 2018, 4:48 PM
jfb committed rL333325: Add nonnull; use it for atomics.
Add nonnull; use it for atomics
May 25 2018, 4:48 PM
jfb closed D47225: Add nonnull; use it for atomics.
May 25 2018, 4:47 PM
jfb committed rL333317: Fix optional<char> test breakage.
Fix optional<char> test breakage
May 25 2018, 2:36 PM
jfb committed rCXX333317: Fix optional<char> test breakage.
Fix optional<char> test breakage
May 25 2018, 2:36 PM
jfb committed rL333315: Fix array deduction guide test breakage.
Fix array deduction guide test breakage
May 25 2018, 2:21 PM
jfb committed rCXX333315: Fix array deduction guide test breakage.
Fix array deduction guide test breakage
May 25 2018, 2:21 PM
jfb committed rCXX333308: Fix optional deduction guide test breakage.
Fix optional deduction guide test breakage
May 25 2018, 1:49 PM
jfb committed rL333308: Fix optional deduction guide test breakage.
Fix optional deduction guide test breakage
May 25 2018, 1:47 PM
jfb added 1 commit(s) for D47229: Make atomic non-member functions as nonnull: rL333290: Follow-up fix for nonnull atomic non-member functions.
May 25 2018, 10:41 AM
jfb added an edge to rL333290: Follow-up fix for nonnull atomic non-member functions: D47229: Make atomic non-member functions as nonnull.
May 25 2018, 10:41 AM
jfb added a comment to D47229: Make atomic non-member functions as nonnull.

Fixed by r333290.

May 25 2018, 10:41 AM
jfb committed rL333290: Follow-up fix for nonnull atomic non-member functions.
Follow-up fix for nonnull atomic non-member functions
May 25 2018, 10:41 AM
jfb committed rC333290: Follow-up fix for nonnull atomic non-member functions.
Follow-up fix for nonnull atomic non-member functions
May 25 2018, 10:40 AM
jfb added a comment to D47229: Make atomic non-member functions as nonnull.

This is causing breaks in fuchsia,

Code that looks like this

uintptr_t last_unlogged =
     atomic_load_explicit(&unlogged_tail, memory_order_acquire);
 do { 
     if (last_unlogged == 0)
         return;
 } while (!atomic_compare_exchange_weak_explicit(&unlogged_tail,
                                                 &last_unlogged, 0,
                                                 memory_order_acq_rel,
                                                 memory_order_relaxed));

Where unlogged_tail is somewhere on the stack. And 'atomic_compare_exchange_weak_explicit' is an #define alias for '__c11_atomic_compare_exchange_weak'

Full context here: https://fuchsia.googlesource.com/zircon/+/master/third_party/ulib/musl/ldso/dynlink.c#822

May 25 2018, 10:15 AM

May 24 2018

jfb added a comment to D47225: Add nonnull; use it for atomics.

Ping! clang side landed in https://reviews.llvm.org/rL333246

May 24 2018, 5:13 PM
jfb committed rC333246: Make atomic non-member functions as nonnull.
Make atomic non-member functions as nonnull
May 24 2018, 5:11 PM
jfb committed rL333246: Make atomic non-member functions as nonnull.
Make atomic non-member functions as nonnull
May 24 2018, 5:11 PM
jfb closed D47229: Make atomic non-member functions as nonnull.
May 24 2018, 5:11 PM
jfb added inline comments to D47229: Make atomic non-member functions as nonnull.
May 24 2018, 4:51 PM
jfb updated the diff for D47229: Make atomic non-member functions as nonnull.
  • Address nit.
  • Change suggested by Richard
May 24 2018, 4:50 PM
jfb added inline comments to D47290: [Sema] -Wformat-pedantic only for NSInteger/NSUInteger %zu/%zi on Darwin.
May 24 2018, 1:14 PM
jfb updated the diff for D47290: [Sema] -Wformat-pedantic only for NSInteger/NSUInteger %zu/%zi on Darwin.
  • Merge format-size-spec-nsinteger
May 24 2018, 1:13 PM
jfb added a comment to D47229: Make atomic non-member functions as nonnull.

Addressed nit.

May 24 2018, 11:38 AM
jfb updated the diff for D47229: Make atomic non-member functions as nonnull.
  • Address nit.
May 24 2018, 11:37 AM
jfb added a comment to D46084: [Fixed Point Arithmetic] Addition of the Fixed Point _Accum type.

Can you also add a test for _Bool _Accum.

May 24 2018, 11:00 AM · Restricted Project

May 23 2018

jfb updated the summary of D47290: [Sema] -Wformat-pedantic only for NSInteger/NSUInteger %zu/%zi on Darwin.
May 23 2018, 3:42 PM
jfb added inline comments to D47290: [Sema] -Wformat-pedantic only for NSInteger/NSUInteger %zu/%zi on Darwin.
May 23 2018, 3:28 PM
jfb created D47290: [Sema] -Wformat-pedantic only for NSInteger/NSUInteger %zu/%zi on Darwin.
May 23 2018, 3:07 PM
jfb added a comment to D46084: [Fixed Point Arithmetic] Addition of the Fixed Point _Accum type.

Actually, scratch that. We will be enabling it since GCC does. Will update this and other relevant C++ related code appropriately.

May 23 2018, 11:34 AM · Restricted Project