Page MenuHomePhabricator

brooksmoses (Brooks Moses)
User

Projects

User does not belong to any projects.

User Details

User Since
Dec 15 2017, 2:00 PM (258 w, 3 d)

Recent Activity

Sep 7 2022

brooksmoses added a comment to D132081: [libc++] Remove __deque_base.

Thanks, @philnik ; that patch indeed works. I appreciate the quick reply, too.

Sep 7 2022, 12:22 AM · Restricted Project, Restricted Project

Sep 6 2022

brooksmoses added a comment to D132081: [libc++] Remove __deque_base.

I'm seeing new compilation errors from the moved allocator static_assert in this patch.

Sep 6 2022, 10:54 AM · Restricted Project, Restricted Project

Aug 10 2022

brooksmoses added a comment to D131351: [C] Default implicit function pointer conversions diagnostic to be an error.

For the record, so far we've seen this showing up in the following:

  • A case in a notoriously warning-heavy third-party library where we'd backported a file from a newer version and didn't quite fix all the internal API mismatches.
  • A ten-year-old bug in a local patch to another third-party library, where a function-pointer parameter was defined as returning a void and then assigned to a function-pointer that returns a void *.
  • A probably-innocuous bug in a local patch to yet another third-party library, where we were passing an int foo(char *, char *) function to GLIBC's qsort, which expects a function with a signature of int foo(void *, void *).
  • A case where https://gitlab.freedesktop.org/pixman/pixman/-/commit/e0d4403e78a7af8f4be4110ae25e9c3d1ac93a78 wasn't applied to our local version. This is also probably an innocuous case, not a "real" bug.
  • A case where SciPy's extension has a function that uses void * for a FILE * pointer (https://github.com/scipy/scipy/blob/main/scipy/spatial/_qhull.pyx#L187, second argument) while the corresponding C code's function has a real FILE * pointer (https://github.com/qhull/qhull/blob/master/src/libqhull_r/io_r.h#L97). The SciPy function also uses a void * for an argument of a struct type, which seems rather odd to me given that it just defined the type two lines earlier.
Aug 10 2022, 10:18 PM · Restricted Project, Restricted Project, Restricted Project

Jul 19 2022

brooksmoses accepted D129899: Bazel BUILD file for BOLT.
Jul 19 2022, 3:55 PM · Restricted Project, Restricted Project

Jul 2 2022

brooksmoses added a comment to D128058: [mlir][sparse] add more unittest cases to sparse dialect merger.

@wrengr : Looks fixed; thanks!

Jul 2 2022, 2:31 PM · Restricted Project, Restricted Project

Jul 1 2022

brooksmoses added a comment to D128058: [mlir][sparse] add more unittest cases to sparse dialect merger.

I'm seeing test warning/errors from this:

Jul 1 2022, 2:09 PM · Restricted Project, Restricted Project

Jun 29 2022

brooksmoses added a comment to D128694: [lldb/Dataformatters] Adapt C++ std::string dataformatter for D128285.

We're seeing a number of LLDB test failures from this change. If I'm understanding correctly, that's working by design because we're building the test code with a version of LLVM that's from a revision that's not quite as recent as the LLDB code we're testing.

Jun 29 2022, 8:48 PM · Restricted Project, Restricted Project

Feb 25 2022

brooksmoses added a comment to rG87753cebf5f8: [X86] combineX86ShufflesRecursively - don't both widening inputs before calling….

Thanks much! We'll get you a test case as soon as we can.

Feb 25 2022, 1:26 AM
brooksmoses raised a concern with rG87753cebf5f8: [X86] combineX86ShufflesRecursively - don't both widening inputs before calling….

As a heads-up: We don't have a reduced testcase yet, but we're seeing some significant errors in an FFT test that root-cause to this change.

Feb 25 2022, 12:30 AM

Apr 9 2021

brooksmoses added a comment to D99790: [CGCall] Annotate `this` argument with alignment.

In any case, thanks for the quick reply, and I'll figure out a small reproducer if we find something that isn't UB.

Nono, you misunderstand, i want the samples *with* UB.
I will then revert this, and add UBSan check to catch that UB first.

Apr 9 2021, 11:45 AM · Restricted Project, Restricted Project

Apr 8 2021

brooksmoses added a comment to D99790: [CGCall] Annotate `this` argument with alignment.

As a heads up, I'm seeing segfaults on internal code as a result of this change, as well as errors in Eigen's unalignedassert.cpp test (specifically, this line asserts: https://github.com/madlib/eigen/blob/master/test/unalignedassert.cpp#L151).

Would be good to have a small standalone reproducer.
Not really sure how we can end up with a misaligned this, but it sounds like UB.

Apr 8 2021, 6:18 PM · Restricted Project, Restricted Project
brooksmoses added a comment to D99790: [CGCall] Annotate `this` argument with alignment.

As a heads up, I'm seeing segfaults on internal code as a result of this change, as well as errors in Eigen's unalignedassert.cpp test (specifically, this line asserts: https://github.com/madlib/eigen/blob/master/test/unalignedassert.cpp#L151).

Apr 8 2021, 2:21 PM · Restricted Project, Restricted Project
brooksmoses added a comment to D75844: [clang] Set begin loc on GNU attribute parsed attrs.

FWIW, this now causes Clang to produce an error on this code, when it didn't before:

Apr 8 2021, 11:31 AM · Restricted Project

Nov 21 2020

brooksmoses added a comment to D17993: [CodeGen] Apply 'nonnull' and 'dereferenceable(N)' to 'this' pointer arguments..

Aha, okay. I hadn't realized that this optimization had a -fno-delete-null-pointer-checks option to disable it. I agree that since that's available there's no call for a rollback.

Nov 21 2020, 4:15 PM · Restricted Project, Restricted Project

Nov 20 2020

brooksmoses added a comment to D17993: [CodeGen] Apply 'nonnull' and 'dereferenceable(N)' to 'this' pointer arguments..

So, I have bad news: This causes OpenJDK to segfault. The relevant code is here:
https://github.com/openjdk/jdk/blob/master/src/hotspot/share/memory/arena.cpp#L311

Nov 20 2020, 11:30 PM · Restricted Project, Restricted Project

Nov 19 2020

brooksmoses added a comment to D90554: [CostModel] remove cost-kind predicate for intrinsics in basic TTI implementation.

As a heads-up: This is causing a lot of Clang segfaults in Google's builds with sanitizers enabled. We're working work on a reduced testcase, but wanted to let you know while we do that.

Nov 19 2020, 5:06 PM · Restricted Project

Jul 10 2020

brooksmoses added a comment to D83440: [InstSimplify] Re-enable select ?, undef, X -> X transform when X is provably not poison.

Yes, it may hide some warnings.
But that's life. Optimizations do hide programming errors. This is not the only one for sure. But it's ok as you are checking the code that will run. If the compiler can "fix" bugs automatically, then developers don't need to worry about those. As long as you keep running these checks continuously to track changes in the compiler like this one :)

Jul 10 2020, 2:05 AM · Restricted Project
brooksmoses added a comment to D83440: [InstSimplify] Re-enable select ?, undef, X -> X transform when X is provably not poison.

Replying to my own question, as I was able to test this sooner than I expected: Yes, it looks like the new MSAN warnings remain after this revision. Excellent! I think that proves that this was a useful fix. :)

Jul 10 2020, 1:53 AM · Restricted Project
brooksmoses added a comment to D83440: [InstSimplify] Re-enable select ?, undef, X -> X transform when X is provably not poison.

I'd erroneously made this comment on the revision (where I think nobody will see it) rather than here, so copying it here:

Jul 10 2020, 1:41 AM · Restricted Project
brooksmoses added a comment to rG469da663f2df: [InstSimplify] Re-enable select ?, undef, X -> X transform when X is provably….

A question about this: We got a couple of dozen MSAN warnings in the Google codebase from the revision that removed these -- I'm guessing that perhaps what happened was that the undef in question was a use-of-uninitialized-value, and this optimization was hiding the use so the MSAN checks didn't trigger. Is re-enabling this going to make those MSAN warnings go away again, re-hiding this undefined behavior?

Jul 10 2020, 1:32 AM

Aug 9 2019

brooksmoses added a comment to rL368479: [X86] Remove custom handling for extloads from LowerLoad..

This breaks the build with an unused-variable warning:

Aug 9 2019, 1:58 PM

Jun 14 2019

brooksmoses added a comment to rL363474: adding more fmf propagation for selects plus tests.

Hey, so I'm seeing these tests starting to fail with llc errors. The output looks like this (after going through the output-scanning layer):

Jun 14 2019, 7:15 PM

Jun 7 2019

brooksmoses added a comment to D62223: [DAGCombiner][X86][AArch64][AMDGPU] (x + C) - y -> (x - y) + C fold.

Unfortunately, we (Google) are seeing some regressions on our internal benchmarks on x86_64 -- mostly around 2%, but in some cases a good bit more -- from this revision and some of the subsequent revisions in the series, notably r362217. Just wanted to give you a heads-up at this point -- we're still working to try to understand the underlying reasons and get a reproducing test-case, but it looks like these are going to block our internal release until they're addressed.

Jun 7 2019, 4:20 PM · Restricted Project

Jun 6 2019

brooksmoses added a comment to rL362539: Revert r362472 as it is breaking PPC build bots.

For what it's worth, I was also seeing some run-time errors on x86 coming from the change that this reverts. I haven't dug into things to track them down, beyond determining that this change makes them go away again.

Jun 6 2019, 1:07 AM

May 22 2019

brooksmoses added a comment to D62249: RegAllocFast: Set MayLiveAcrossBlocks when allocating uses.

Thanks for the quick fix for bug 41973!

May 22 2019, 11:50 AM

Jun 9 2018

brooksmoses added a comment to D47975: [ELF] Fix copy relocation when two symbols share the same Symbol instance..

While you're doing that, maybe also fix "refer different places" in the comment on line 447 to be "refer to different places". :)

Jun 9 2018, 2:01 AM

Apr 18 2018

brooksmoses added a comment to D45685: [Sema] Add -wtest global flag that silences -Wself-assign for overloaded operators..

Thanks for the summary, John. To confirm, I found two examples of bugs involving local variables, as well as the field-based examples. And, indeed, all of the false positives were in unit tests.

Apr 18 2018, 10:22 AM · Restricted Project

Apr 16 2018

brooksmoses added a comment to D44883: [Sema] Extend -Wself-assign and -Wself-assign-field to warn on overloaded self-assignment (classes).

A further concern about this in the general case from the reviewer of one of my test-cleanup changes: The "var = *&var" idiom is not necessarily equivalent to "var = var" in cases of user-defined types, because operator& may be overloaded.

Apr 16 2018, 2:06 AM
brooksmoses added a comment to D44883: [Sema] Extend -Wself-assign and -Wself-assign-field to warn on overloaded self-assignment (classes).

Some further statistics, now that I've done a full cleanup on our code:

Apr 16 2018, 1:16 AM

Apr 15 2018

brooksmoses added a comment to D44883: [Sema] Extend -Wself-assign and -Wself-assign-field to warn on overloaded self-assignment (classes).

Further note: I'm noticing that nearly all the signal is from -Wself-assign-field and all the noise is from -Wself-assign. So that would be one obvious way to make this higher-signal in what's enabled in -Wall, if that were a desire.

Apr 15 2018, 4:56 PM
brooksmoses added a comment to D44883: [Sema] Extend -Wself-assign and -Wself-assign-field to warn on overloaded self-assignment (classes).

I have noticed two things when attempting to release LLVM with this revision internally at Google:

Apr 15 2018, 3:09 PM

Dec 15 2017

brooksmoses added a comment to D39407: [(new) Pass Manager] instantiate SimplifyCFG with the same options as the old PM.

We're seeing about 10% regressions in an internal benchmark as a result of this change. Still digging further, but wanted to send a heads-up.

Dec 15 2017, 2:02 PM