Page MenuHomePhabricator

jfb (JF Bastien)
User

Projects

User does not belong to any projects.

User Details

User Since
Jan 27 2014, 9:36 AM (264 w, 4 d)

Recent Activity

Thu, Feb 21

jfb added inline comments to D58251: Extend "idempotent" atomicrmw optimizations to floating point.
Thu, Feb 21, 10:05 AM

Fri, Feb 15

jfb added inline comments to D58201: Make std::memory_order an enum class (P0439R0).
Fri, Feb 15, 10:43 AM
jfb updated subscribers of D58188: CodeGen: Explicitly initialize structure padding in the -ftrivial-auto-var-init mode.

Overall this LGTM, but it would be great to get @pcc to double-check. He's got way more attention to details than I do :)

Fri, Feb 15, 10:26 AM
jfb added a comment to D57820: [AArch64] Use CAS loops for all atomic operations when available..

Incidentally the handling of volatile atomic operations here is probably wrong as I don't think we can have any kind of repeated loading, but that's nothing to do with this change (both LDXR/STXR loop and CAS loop have this problem) and doesn't need to be fixed by it.

Fri, Feb 15, 10:11 AM · Restricted Project
jfb accepted D58290: Convert atomicrmws to xchg or store where legal.

A few nits, LGTM otherwise.

Fri, Feb 15, 9:55 AM · Restricted Project
jfb committed rG0bae08ae7698: Variable auto-init of blocks capturing self after init bugfix (authored by jfb).
Variable auto-init of blocks capturing self after init bugfix
Fri, Feb 15, 9:27 AM
jfb committed rC354147: Variable auto-init of blocks capturing self after init bugfix.
Variable auto-init of blocks capturing self after init bugfix
Fri, Feb 15, 9:26 AM
jfb committed rL354147: Variable auto-init of blocks capturing self after init bugfix.
Variable auto-init of blocks capturing self after init bugfix
Fri, Feb 15, 9:26 AM
jfb closed D58218: Variable auto-init of blocks capturing self after init bugfix.
Fri, Feb 15, 9:26 AM · Restricted Project, Restricted Project

Thu, Feb 14

jfb updated the diff for D58218: Variable auto-init of blocks capturing self after init bugfix.
  • Fix polarity
Thu, Feb 14, 5:05 PM · Restricted Project, Restricted Project
jfb added a comment to D58218: Variable auto-init of blocks capturing self after init bugfix.
In D58218#1398124, @jfb wrote:

Well, you can always make a variable with a more directly-applicable name than capturedByInit and update it as appropriate, like locIsByrefHeader.

Sounds good. I made it const too, to avoid inadvertent modification.

This is actually the reverse of the sense I would expect it to have based on the name. If you want these semantics, maybe locIsObject?

Thu, Feb 14, 5:05 PM · Restricted Project, Restricted Project
jfb added a comment to D58188: CodeGen: Explicitly initialize structure padding in the -ftrivial-auto-var-init mode.

This is a good start!

Thu, Feb 14, 3:29 PM
jfb updated subscribers of D58251: Extend "idempotent" atomicrmw optimizations to floating point.
Thu, Feb 14, 1:23 PM
jfb updated the summary of D58164: Block+lambda: allow reference capture.
Thu, Feb 14, 12:48 PM · Restricted Project
jfb accepted D58244: Canonicalize all "idempotent" atomicrmw ops.
Thu, Feb 14, 12:21 PM · Restricted Project
jfb requested changes to D58244: Canonicalize all "idempotent" atomicrmw ops.
Thu, Feb 14, 11:04 AM · Restricted Project
jfb accepted D58242: Teach instcombine about remaining idemptotent atomicrmw types.

For the idempotent RMW, atomicrmw or, ptr, 0 sgtm. I assume that you'd do the same for floating-point (with a cast)? I wonder if in the long run we'd actually just want a separate instruction for this...

Thu, Feb 14, 10:40 AM · Restricted Project
jfb updated the diff for D58218: Variable auto-init of blocks capturing self after init bugfix.
  • Update with John's suggestion.
Thu, Feb 14, 9:01 AM · Restricted Project, Restricted Project
jfb added a comment to D58218: Variable auto-init of blocks capturing self after init bugfix.

Well, you can always make a variable with a more directly-applicable name than capturedByInit and update it as appropriate, like locIsByrefHeader.

Thu, Feb 14, 9:01 AM · Restricted Project, Restricted Project
jfb added inline comments to D58201: Make std::memory_order an enum class (P0439R0).
Thu, Feb 14, 8:44 AM

Wed, Feb 13

jfb added a comment to D58149: [clang] Make sure C99/C11 features in <float.h> are provided in C++11.
In D58149#1397390, @jfb wrote:

Formally, I don't think C11 is a normative reference for C++11 or C++14, only C++17 (see [intro.refs] in the standard).

You're right. I don't mind submitting a patch that enables it for C++17 and above only if that's what you want, however...

Right, this was changed in wg21.link/p0063r3
That being said, we *can* offer these C11 features as extensions IIUC.

... I think it's also fine to have it in C++11.

In C++11, this is technically valid:

#define FLT_TRUE_MIN 1
#include <cfloat>
#if FLT_TRUE_MIN != 1
#error float.h redefined my macro
#endif

and likewise

#include <cfloat>
constexpr float FLT_TRUE_MIN = 0.0001f;

... but both will fail with this patch, so providing these macros is technically non-conforming. I don't know to what extent we should care about such uses, though. We generally leak additional non-conforming names from the system <*.h> headers into the user's namespace if the libc headers put them there. This would, however, probably be the first time that we put them there ourselves.

Wed, Feb 13, 9:11 PM · Restricted Project
jfb added a comment to D58201: Make std::memory_order an enum class (P0439R0).

Two small comments, LGTM otherwise (but would like one of the libc++ maintainers to sign off too).

Wed, Feb 13, 9:09 PM
jfb added inline comments to D58218: Variable auto-init of blocks capturing self after init bugfix.
Wed, Feb 13, 8:53 PM · Restricted Project, Restricted Project
jfb created D58218: Variable auto-init of blocks capturing self after init bugfix.
Wed, Feb 13, 4:48 PM · Restricted Project, Restricted Project
jfb added a comment to D58149: [clang] Make sure C99/C11 features in <float.h> are provided in C++11.

Formally, I don't think C11 is a normative reference for C++11 or C++14, only C++17 (see [intro.refs] in the standard).

Wed, Feb 13, 3:59 PM · Restricted Project
jfb added inline comments to D58201: Make std::memory_order an enum class (P0439R0).
Wed, Feb 13, 3:13 PM
jfb added a comment to D58164: Block+lambda: allow reference capture.

I talked to Akira in meatspace, and it seems like this updated patch does the right thing. He suggested changing the AST as a longer-term solution, but for now this approach seems simple enough.

Wed, Feb 13, 3:08 PM · Restricted Project
jfb updated the diff for D58164: Block+lambda: allow reference capture.
  • Check for references when looking for copyexpr directly.
Wed, Feb 13, 3:07 PM · Restricted Project
jfb added inline comments to D58201: Make std::memory_order an enum class (P0439R0).
Wed, Feb 13, 3:02 PM
jfb updated subscribers of D58201: Make std::memory_order an enum class (P0439R0).
Wed, Feb 13, 3:02 PM
jfb added a comment to D58188: CodeGen: Explicitly initialize structure padding in the -ftrivial-auto-var-init mode.

Oh this is great, looking forward to it!

Wed, Feb 13, 9:04 AM
jfb added inline comments to D57898: CodeGen: Fix PR40605: split constant structures generated by -ftrivial-auto-var-init when emitting initializators.
Wed, Feb 13, 9:00 AM
jfb added a comment to D57898: CodeGen: Fix PR40605: split constant structures generated by -ftrivial-auto-var-init when emitting initializators.

... which happily skips the padding.

Wed, Feb 13, 8:59 AM

Tue, Feb 12

jfb created D58164: Block+lambda: allow reference capture.
Tue, Feb 12, 9:35 PM · Restricted Project
jfb accepted D58149: [clang] Make sure C99/C11 features in <float.h> are provided in C++11.
Tue, Feb 12, 2:43 PM · Restricted Project
jfb committed rG09197775c51e: [NFC] typo (authored by jfb).
[NFC] typo
Tue, Feb 12, 12:20 PM
jfb committed rL353875: [NFC] typo.
[NFC] typo
Tue, Feb 12, 12:20 PM
jfb committed rC353875: [NFC] typo.
[NFC] typo
Tue, Feb 12, 12:20 PM
jfb added a comment to D57898: CodeGen: Fix PR40605: split constant structures generated by -ftrivial-auto-var-init when emitting initializators.

Overall this LGTM besides a few nits, and wanting input from @rjmccall.

Tue, Feb 12, 9:32 AM

Mon, Feb 11

jfb added a comment to D57761: [libc++] Avoid UB in the no-exceptions mode in a few places.

The tests are exactly what I had in mind, great! LGTM.

Mon, Feb 11, 1:13 PM · Restricted Project

Sat, Feb 9

jfb added a comment to D58004: Bit-casting object representations (p0476r2).

No, because it wouldn't support everything the standard says. Things like float -> int are also wrong with what you suggest.

Fair enough. Out of curiosity though, what is wrong with float -> int?

Sat, Feb 9, 12:15 PM
jfb added a comment to D58004: Bit-casting object representations (p0476r2).

For sure, but would it work instead of a builtin?

Sat, Feb 9, 11:59 AM
jfb added a comment to D58004: Bit-casting object representations (p0476r2).

Here is another idea, what if we treated fundamental types differently because they can be cast using c-style casting? Like this:

     std::is_fundamental<_To>::value &&
     std::is_fundamental<_Fm>::value,
     _To
>::type
 bit_cast(const _Fm &__src) noexcept
 {
     return (_To) __src;
 }
Sat, Feb 9, 11:56 AM
jfb added a comment to D44248: [clangd][cmake] Provide libatomic when there is no native support for 64bit atomics.

This appears to be adding atomic on macOS builds, which shouldn't be happening:

https://hydra.nixos.org/build/88290234/

HAVE_CXX_ATOMICS64_WITHOUT_LIB is never being set.

Sat, Feb 9, 11:34 AM · Restricted Project
jfb requested changes to D58004: Bit-casting object representations (p0476r2).

You need support from the compiler for constexpr support. I don't think we want this without constexpr. @erik.pilkington was looking into this.

Sat, Feb 9, 11:31 AM

Fri, Feb 8

jfb added a comment to D57990: Use std::make_shared in LLDB (NFC).

Do you ever use weak pointers? The single allocation is good until you start holding on to more things (because you can't just keep the block around, you need the entire allocation), so you should make sure you won’t be delaying memory being reclaimed.

Fri, Feb 8, 9:10 PM · Restricted Project
jfb added a comment to D57898: CodeGen: Fix PR40605: split constant structures generated by -ftrivial-auto-var-init when emitting initializators.

Can you add a link to bug 40605 in the commit message?

It's in the title now, doesn't that count? :)

Fri, Feb 8, 10:22 AM
jfb added a comment to D57898: CodeGen: Fix PR40605: split constant structures generated by -ftrivial-auto-var-init when emitting initializators.

Can you add a link to bug 40605 in the commit message?

Fri, Feb 8, 10:04 AM
jfb added a comment to rL353490: [NFC] Variable auto-init: use getAsVariableArrayType helper.

LGTM, but why did I just get like a million different notifications of this from different services.

Fri, Feb 8, 9:35 AM

Thu, Feb 7

jfb committed rGb347e7525809: Variable auto-init: fix __block initialization (authored by jfb).
Variable auto-init: fix __block initialization
Thu, Feb 7, 5:29 PM
jfb committed rL353495: Variable auto-init: fix __block initialization.
Variable auto-init: fix __block initialization
Thu, Feb 7, 5:29 PM
jfb committed rC353495: Variable auto-init: fix __block initialization.
Variable auto-init: fix __block initialization
Thu, Feb 7, 5:29 PM
jfb closed D57797: Variable auto-init: fix __block initialization.
Thu, Feb 7, 5:29 PM · Restricted Project
jfb updated the diff for D57797: Variable auto-init: fix __block initialization.
  • Remove whitespace changes.
Thu, Feb 7, 4:59 PM · Restricted Project
jfb updated the summary of D57797: Variable auto-init: fix __block initialization.
Thu, Feb 7, 4:53 PM · Restricted Project
jfb added inline comments to D57797: Variable auto-init: fix __block initialization.
Thu, Feb 7, 4:52 PM · Restricted Project
jfb committed rGddeb2f2a1605: [NFC] Variable auto-init: use getAsVariableArrayType helper (authored by jfb).
[NFC] Variable auto-init: use getAsVariableArrayType helper
Thu, Feb 7, 4:51 PM
jfb committed rL353490: [NFC] Variable auto-init: use getAsVariableArrayType helper.
[NFC] Variable auto-init: use getAsVariableArrayType helper
Thu, Feb 7, 4:51 PM
jfb committed rC353490: [NFC] Variable auto-init: use getAsVariableArrayType helper.
[NFC] Variable auto-init: use getAsVariableArrayType helper
Thu, Feb 7, 4:51 PM
jfb updated the diff for D57797: Variable auto-init: fix __block initialization.
  • Simplify patch greatly.
Thu, Feb 7, 4:51 PM · Restricted Project
jfb added inline comments to D57797: Variable auto-init: fix __block initialization.
Thu, Feb 7, 3:59 PM · Restricted Project
jfb added inline comments to D57797: Variable auto-init: fix __block initialization.
Thu, Feb 7, 3:55 PM · Restricted Project
jfb added inline comments to D57797: Variable auto-init: fix __block initialization.
Thu, Feb 7, 3:48 PM · Restricted Project
jfb updated the diff for D57797: Variable auto-init: fix __block initialization.
  • Only initialize __block's storage.
Thu, Feb 7, 3:48 PM · Restricted Project
jfb updated subscribers of D57264: Bump minimum toolchain version.

Regarding libstdc++: the check is currently conservatively correct, but not thorough enough, because older revisions of GCC still get bug fix release which pass the date check. Past GCC 7 we can use _GLIBCXX_RELEASE, but before this @tstellar points out how LLDB does it. I don't know if we have to fix this issue: we simply fail to warn a small subset of libstdc++ users.

Thu, Feb 7, 2:08 PM · Restricted Project
jfb updated subscribers of D57898: CodeGen: Fix PR40605: split constant structures generated by -ftrivial-auto-var-init when emitting initializators.
Thu, Feb 7, 12:48 PM
jfb added a comment to D57898: CodeGen: Fix PR40605: split constant structures generated by -ftrivial-auto-var-init when emitting initializators.

Thanks for taking this on! As I said in the bug I think we definitely want something like this change.

Thu, Feb 7, 12:45 PM
jfb added a comment to rL353374: Bump minimum toolchain version.

This looks to have broken optimized tablegen builds with VS2015 even when the new switch is enabled. I'm guessing that option isn't passed into the cmake run for generating the optimized tablegen?

Can you clarify what exactly is broken? Is there a bot with logs?
The new minimum VS version is 2017, so indeed something looks wrong.

I discovered this when trying to build LLVM locally today. I still use VS2015, which should work if the cmake variable for allowing building with an old toolchain is set. I set it, but discovered that the cmake step generating optimized tablegen still fails with the soft error.

Thu, Feb 7, 10:54 AM
jfb added inline comments to D57854: [InstCombine] Optimize `atomicrmw <op>, 0` into `load atomic` when possible.
Thu, Feb 7, 10:31 AM · Restricted Project
jfb added a comment to rL353374: Bump minimum toolchain version.

This looks to have broken optimized tablegen builds with VS2015 even when the new switch is enabled. I'm guessing that option isn't passed into the cmake run for generating the optimized tablegen?

Thu, Feb 7, 10:24 AM

Wed, Feb 6

jfb added a comment to D57264: Bump minimum toolchain version.

Looks like clang-with-lto-ubuntu is still on an old version of GCC. Let's see if more bots are broken, I'm not sure how many stragglers we have.

Wed, Feb 6, 10:19 PM · Restricted Project
jfb committed rG388cefa78d4d: Bump minimum toolchain version (authored by jfb).
Bump minimum toolchain version
Wed, Feb 6, 9:20 PM
jfb committed rL353374: Bump minimum toolchain version.
Bump minimum toolchain version
Wed, Feb 6, 9:20 PM
jfb added a comment to D57264: Bump minimum toolchain version.
In D57264#1385384, @jfb wrote:
In D57264#1383646, @jfb wrote:
In D57264#1381951, @jfb wrote:

Reverted for (I hope) the last time because of:

polly-amd64-linux
polly-arm-linux

They both have old versions of libstdc++ and recent clang. Once fixed the toolchain bump should stick.

I haven't heard back from bot owners. I'll opt them out of the toolchain checks for now: https://reviews.llvm.org/D57701

The bots haven't updated yet, I sent a ping to @gkistanova, @grosser and @adasgupt.

Wed, Feb 6, 9:19 PM · Restricted Project
jfb closed D57264: Bump minimum toolchain version.
Wed, Feb 6, 9:19 PM · Restricted Project
jfb accepted D57854: [InstCombine] Optimize `atomicrmw <op>, 0` into `load atomic` when possible.

One fix then this LGTM

Wed, Feb 6, 4:47 PM · Restricted Project
jfb added a comment to D57865: [zorg] Update host compiler for polly-arm-linux builder.

Thanks for doing this!

Wed, Feb 6, 4:35 PM
jfb added a comment to D57854: [InstCombine] Optimize `atomicrmw <op>, 0` into `load atomic` when possible.

FWIW you might find wg21.link/n4455 relevant :)

Wed, Feb 6, 4:23 PM · Restricted Project
jfb added inline comments to D57854: [InstCombine] Optimize `atomicrmw <op>, 0` into `load atomic` when possible.
Wed, Feb 6, 4:21 PM · Restricted Project
jfb added inline comments to D57854: [InstCombine] Optimize `atomicrmw <op>, 0` into `load atomic` when possible.
Wed, Feb 6, 3:14 PM · Restricted Project
jfb added inline comments to D57797: Variable auto-init: fix __block initialization.
Wed, Feb 6, 3:00 PM · Restricted Project

Tue, Feb 5

jfb updated the summary of D57797: Variable auto-init: fix __block initialization.
Tue, Feb 5, 5:05 PM · Restricted Project
jfb created D57797: Variable auto-init: fix __block initialization.
Tue, Feb 5, 5:03 PM · Restricted Project
jfb added a comment to D57761: [libc++] Avoid UB in the no-exceptions mode in a few places.
In D57761#1385562, @jfb wrote:

I'm wondering if, as a follow-up, the XFAIL tests that validate the behavior for exceptions should instead:

  • Not XFAIL
  • Install a SIGABORT handler, and call exit(0) or something
  • Instead of catch, call exit(1) because we expected the abort handler to be called.

    That type of testing will find any missing fixes from this patch (and prevent regressions).
Tue, Feb 5, 10:58 AM · Restricted Project
jfb added a comment to D57761: [libc++] Avoid UB in the no-exceptions mode in a few places.

I'm wondering if, as a follow-up, the XFAIL tests that validate the behavior for exceptions should instead:

Tue, Feb 5, 10:30 AM · Restricted Project
jfb added a reviewer for D57761: [libc++] Avoid UB in the no-exceptions mode in a few places: jfb.
Tue, Feb 5, 10:03 AM · Restricted Project
jfb updated subscribers of D57264: Bump minimum toolchain version.
In D57264#1383646, @jfb wrote:
In D57264#1381951, @jfb wrote:

Reverted for (I hope) the last time because of:

polly-amd64-linux
polly-arm-linux

They both have old versions of libstdc++ and recent clang. Once fixed the toolchain bump should stick.

I haven't heard back from bot owners. I'll opt them out of the toolchain checks for now: https://reviews.llvm.org/D57701

Tue, Feb 5, 9:23 AM · Restricted Project

Mon, Feb 4

jfb committed rG73f499771f84: Fix double curlies (authored by jfb).
Fix double curlies
Mon, Feb 4, 9:34 PM
jfb added a comment to D57624: Support tests in freestanding.

Hi @jfb, this commit broke /utils/generate_feature_test_macro_components.py in the libc++ repo by replacing two curly braces with one curly brace:

diff --git a/utils/generate_feature_test_macro_components.py b/utils/generate_feature_test_macro_components.py
index 2bd80a8..d923208 100755
--- a/utils/generate_feature_test_macro_components.py
+++ b/utils/generate_feature_test_macro_components.py
@@ -865,7 +865,7 @@ def produce_tests():
 
 #endif // TEST_STD_VER > 17
 
-int main() {{}}
+int main(int, char**) { return 0; }
 """.format(script_name=script_name,
            header=h,
            test_tags=test_tags,

The correct line here would have been

int main(int, char**) {{ return 0; }}

Do you think you could push that fix? Thanks!

Mon, Feb 4, 9:34 PM · Restricted Project
jfb committed rCXX353140: Fix double curlies.
Fix double curlies
Mon, Feb 4, 9:34 PM
jfb committed rL353140: Fix double curlies.
Fix double curlies
Mon, Feb 4, 9:33 PM
jfb committed rL353086: Support tests in freestanding.
Support tests in freestanding
Mon, Feb 4, 1:02 PM
jfb committed rCXX353086: Support tests in freestanding.
Support tests in freestanding
Mon, Feb 4, 1:01 PM
jfb committed rG2df59c50688c: Support tests in freestanding (authored by jfb).
Support tests in freestanding
Mon, Feb 4, 12:35 PM
jfb closed D57624: Support tests in freestanding.
Mon, Feb 4, 12:34 PM · Restricted Project
jfb committed rL353085: Opt polly amd64 and arm bots out of toolchain checks.
Opt polly amd64 and arm bots out of toolchain checks
Mon, Feb 4, 12:26 PM
jfb closed D57701: Opt polly amd64 and arm bots out of toolchain checks.
Mon, Feb 4, 12:26 PM · Restricted Project
jfb added a comment to D57264: Bump minimum toolchain version.
In D57264#1381951, @jfb wrote:

Reverted for (I hope) the last time because of:

polly-amd64-linux
polly-arm-linux

They both have old versions of libstdc++ and recent clang. Once fixed the toolchain bump should stick.

Mon, Feb 4, 11:30 AM · Restricted Project
jfb created D57701: Opt polly amd64 and arm bots out of toolchain checks.
Mon, Feb 4, 11:29 AM · Restricted Project
jfb added a comment to D57624: Support tests in freestanding.

Like Marshall said, LGTM if all changes look like https://reviews.llvm.org/differential/changeset/?ref=1324002. What's the plan for actually supporting freestanding though? We'll need a separate entry point? Do we know that it's feasible?

Mon, Feb 4, 11:01 AM · Restricted Project