Page MenuHomePhabricator

christof (Christof Douma)
User

Projects

User does not belong to any projects.

User Details

User Since
Oct 9 2015, 6:53 AM (189 w, 1 d)

Recent Activity

Mar 25 2019

christof added an edge to rG8cfd91dcc726: [AArch64] Fix bug 35094 atomicrmw on Armv8.1-A+lse: D58348: [AArch64] Fix for bug 35094 atomicrmw on Armv8.1-A+lse.
Mar 25 2019, 5:03 AM
christof added 1 commit(s) for D58348: [AArch64] Fix for bug 35094 atomicrmw on Armv8.1-A+lse: rG8cfd91dcc726: [AArch64] Fix bug 35094 atomicrmw on Armv8.1-A+lse.
Mar 25 2019, 5:03 AM · Restricted Project

Mar 18 2019

christof closed D58348: [AArch64] Fix for bug 35094 atomicrmw on Armv8.1-A+lse.

Committed as r356360

Mar 18 2019, 2:28 AM · Restricted Project
christof added 1 commit(s) for D58348: [AArch64] Fix for bug 35094 atomicrmw on Armv8.1-A+lse: rL356360: [AArch64] Fix bug 35094 atomicrmw on Armv8.1-A+lse.
Mar 18 2019, 2:28 AM · Restricted Project
christof added an edge to rL356360: [AArch64] Fix bug 35094 atomicrmw on Armv8.1-A+lse: D58348: [AArch64] Fix for bug 35094 atomicrmw on Armv8.1-A+lse.
Mar 18 2019, 2:28 AM
christof committed rG8cfd91dcc726: [AArch64] Fix bug 35094 atomicrmw on Armv8.1-A+lse (authored by christof).
[AArch64] Fix bug 35094 atomicrmw on Armv8.1-A+lse
Mar 18 2019, 2:20 AM
christof committed rL356360: [AArch64] Fix bug 35094 atomicrmw on Armv8.1-A+lse.
[AArch64] Fix bug 35094 atomicrmw on Armv8.1-A+lse
Mar 18 2019, 2:20 AM

Feb 26 2019

christof added a comment to D58348: [AArch64] Fix for bug 35094 atomicrmw on Armv8.1-A+lse.

Is that up for debate in case this is an edge case that is unlikely to be hit?

When it comes to the atomics in general, it's important to allow people to write code with the same performance characteristics as hand-written assembly, or else we force people will go around the compiler using inline asm. But the performance impact here should be basically zero for code that doesn't use an acquire fence, I think, so it shouldn't be an issue here.

Feb 26 2019, 1:59 AM · Restricted Project

Feb 25 2019

christof added inline comments to D58348: [AArch64] Fix for bug 35094 atomicrmw on Armv8.1-A+lse.
Feb 25 2019, 10:19 AM · Restricted Project
christof updated the diff for D58348: [AArch64] Fix for bug 35094 atomicrmw on Armv8.1-A+lse.
Feb 25 2019, 10:19 AM · Restricted Project

Feb 21 2019

christof added a comment to D58348: [AArch64] Fix for bug 35094 atomicrmw on Armv8.1-A+lse.

Are you sure that Bugzilla link is right?

Fixed. The bug number in the subject was the correct one.

Feb 21 2019, 3:38 AM · Restricted Project
christof updated the summary of D58348: [AArch64] Fix for bug 35094 atomicrmw on Armv8.1-A+lse.
Feb 21 2019, 3:21 AM · Restricted Project

Feb 18 2019

christof created D58348: [AArch64] Fix for bug 35094 atomicrmw on Armv8.1-A+lse.
Feb 18 2019, 5:50 AM · Restricted Project

Mar 28 2018

christof committed rL328691: [ARM] Support float literals under XO.
[ARM] Support float literals under XO
Mar 28 2018, 3:05 AM
christof closed D44941: [ARM] Support float literals under XO.
Mar 28 2018, 3:05 AM

Mar 27 2018

christof added a comment to D38792: Fix float literals under XO & fp-armv8.

Follow up patch at https://reviews.llvm.org/D44941

Mar 27 2018, 9:00 AM
christof created D44941: [ARM] Support float literals under XO.
Mar 27 2018, 8:58 AM

Mar 23 2018

christof closed D38792: Fix float literals under XO & fp-armv8.

Committed as r328313

Mar 23 2018, 6:11 AM
christof committed rL328313: [ARM] Support float literals under XO.
[ARM] Support float literals under XO
Mar 23 2018, 6:04 AM

Mar 21 2018

christof updated the diff for D38792: Fix float literals under XO & fp-armv8.

Updated the change to take into account f16 types. I cannot test it yet, as f16 seems to be incomplete. I've reported this to SjoerdMeijer already, and he is fixing f16. I am wondering if we can commit this and once f16 constants are actually lowered, update the test as part of that change.

Mar 21 2018, 8:44 AM

Feb 14 2018

christof added a comment to D42403: libcxx: Move #include_next <math.h> out of header guard in wrapper header..

I've raised https://bugs.llvm.org/show_bug.cgi?id=36382 against this. I think the patch to fix it is trivial.

Feb 14 2018, 7:30 AM

Feb 12 2018

christof accepted D42478: [AArch64] Improve v8.1-A code-gen for atomic load-and.

This looks good to me. Thanks

Feb 12 2018, 8:40 AM
christof accepted D42477: [AArch64] Improve v8.1-A code-gen for atomic load-subtract.

Thanks for the clarification. Looked at the tests and now see what this is doing.
The code looks good to me.
Thanks

Feb 12 2018, 5:49 AM
christof added a comment to D42477: [AArch64] Improve v8.1-A code-gen for atomic load-subtract.

On the previous review (D35375), Geoff Berry suggested to do this in DAG Combine, to see if you can do some further combines, and suggested to not limit this to constants: https://reviews.llvm.org/D35375#810045

Feb 12 2018, 4:45 AM

Jan 26 2018

christof accepted D42104: [AArch64] Generate the CASP instruction for 128-bit cmpxchg.

Code looks well formatted to me, implementation is reasonable to me. It is a bit of a shame that it can't be done in tablegen as the lowering function gets a bit big. But since i128 is not a legal type, I guess this is the best we can get.

Jan 26 2018, 7:26 AM

Oct 11 2017

christof created D38792: Fix float literals under XO & fp-armv8.
Oct 11 2017, 3:07 AM

Aug 4 2017

christof added a comment to D35319: LSE Atomics reorg - Part I.

Thanks for the hard work. It looks good. Quite an exhaustive list of tests as well.

Aug 4 2017, 8:16 AM

Aug 2 2017

christof commandeered D31814: Add option to add command-line options and version info to output file.

With the ok of the author, I'll try to get some movement on this work again.

Aug 2 2017, 8:13 AM

Jul 19 2017

christof accepted D35058: [docs] Document how to debug instruction scheduling model generation.
Jul 19 2017, 2:20 AM

Jul 17 2017

christof added a comment to D35058: [docs] Document how to debug instruction scheduling model generation.

Please make sure that the build directory is not assumed to be present in the root of the source tree. For the rest I'm happy to approve it.

Jul 17 2017, 3:26 AM

Jul 14 2017

christof added a comment to D35375: [AArch64][Atomic] Canonicalize sub of immediate to add of -immediate..

Perhaps this would be better done at the IR level, in InstCombine?

Seems reasonable, but we'd have to know this is a good canonicalization for all targets, right?

It seems like it is to me. Worst case (e.g. the target supports 'add' but not 'sub'), the sub would ideally just be transformed back and a negate added during ISel legalization.

Jul 14 2017, 6:20 AM

Jul 13 2017

christof added inline comments to D35319: LSE Atomics reorg - Part I.
Jul 13 2017, 10:44 AM
christof added inline comments to D35319: LSE Atomics reorg - Part I.
Jul 13 2017, 9:29 AM
christof added a comment to D35319: LSE Atomics reorg - Part I.

Few inline comments on the dead register pass. I've not looked in detail to the other changes done in this patch.

Jul 13 2017, 7:49 AM

Jul 11 2017

christof added a comment to D35058: [docs] Document how to debug instruction scheduling model generation.

Good idea to add a little write-up on that utility. Might want to make it more clear that it is about the machine scheduler models. It does not do anything for the Itinerary based schedule models.

Jul 11 2017, 5:28 AM

Jun 28 2017

christof added a comment to D31709: [NFC] Refactor DiagnosticRenderer to use FullSourceLoc.

Sorry for missing the API comments. Thanks for the fix chapuni :-)

Jun 28 2017, 1:30 AM

Jun 27 2017

christof committed rL306384: Revert "Revert "[NFC] Refactor DiagnosticRenderer to use FullSourceLoc"".
Revert "Revert "[NFC] Refactor DiagnosticRenderer to use FullSourceLoc""
Jun 27 2017, 2:51 AM
christof closed D31709: [NFC] Refactor DiagnosticRenderer to use FullSourceLoc by committing rL306384: Revert "Revert "[NFC] Refactor DiagnosticRenderer to use FullSourceLoc"".
Jun 27 2017, 2:50 AM

Jun 22 2017

christof added a reviewer for D34513: [NFC] Update to account for DiagnosticRenderer use of FullSourceLoc: alexfh.

Added Alexander as he seems to have touched clang-tidy recently.

Jun 22 2017, 7:51 AM
christof added a comment to D34513: [NFC] Update to account for DiagnosticRenderer use of FullSourceLoc.

This looks good to me. But since I have never touch this code, I'll wait a bit till accepting this for others to have a say.

Jun 22 2017, 7:43 AM

Jun 21 2017

christof committed rL305918: [AARCH64][LSE] Preliminary support for ARMv8.1 LSE Atomics..
[AARCH64][LSE] Preliminary support for ARMv8.1 LSE Atomics.
Jun 21 2017, 8:19 AM
christof added a comment to D33586: ARMv8.1 support for LLVM AArch64.

Is it possible that the tests were excluded from the commit?

Jun 21 2017, 8:13 AM
christof committed rL305893: [AARCH64][LSE] Preliminary support for ARMv8.1 LSE Atomics..
[AARCH64][LSE] Preliminary support for ARMv8.1 LSE Atomics.
Jun 21 2017, 3:59 AM
christof closed D33586: ARMv8.1 support for LLVM AArch64 by committing rL305893: [AARCH64][LSE] Preliminary support for ARMv8.1 LSE Atomics..
Jun 21 2017, 3:59 AM
christof added a comment to D33586: ARMv8.1 support for LLVM AArch64.

I can commit it on your behalf if you want me to.

Would greatly appreciate it!

Jun 21 2017, 3:07 AM
christof added a comment to D33586: ARMv8.1 support for LLVM AArch64.

Thank you Chris, I do not have commit rights, should I forward to the mailing list?

Jun 21 2017, 2:56 AM
christof accepted D33586: ARMv8.1 support for LLVM AArch64.

OK, it looks good to me now. Thanks.

Jun 21 2017, 1:43 AM

Jun 19 2017

christof added a comment to D33586: ARMv8.1 support for LLVM AArch64.

Fixed CAS, renamed operands to "old/new" for clarity. Also updated test to use x0/x1 to confirm proper operand order.

Great!

Jun 19 2017, 9:02 AM
christof committed rL305688: Revert "[NFC] Refactor DiagnosticRenderer to use FullSourceLoc".
Revert "[NFC] Refactor DiagnosticRenderer to use FullSourceLoc"
Jun 19 2017, 5:42 AM
christof added a reverting change for rL305684: [NFC] Refactor DiagnosticRenderer to use FullSourceLoc: rL305688: Revert "[NFC] Refactor DiagnosticRenderer to use FullSourceLoc".
Jun 19 2017, 5:42 AM
christof committed rL305684: [NFC] Refactor DiagnosticRenderer to use FullSourceLoc.
[NFC] Refactor DiagnosticRenderer to use FullSourceLoc
Jun 19 2017, 5:06 AM
christof added a comment to D31709: [NFC] Refactor DiagnosticRenderer to use FullSourceLoc.

I'll go ahead and commit this on the grounds that it was already approved and the changes I made were the last nit-picks of @rnk

Jun 19 2017, 5:05 AM
christof accepted D33773: [ARM] llc -arm-execute-only with floating point runs into UNREACHABLE.

The Address Sanitizer caught a stack-use-after-scope of a Twine variable. This is now fixed by passing the Twine directly as a function parameter.

Jun 19 2017, 2:11 AM

Jun 15 2017

christof added a comment to D33586: ARMv8.1 support for LLVM AArch64.

Found something else.

Jun 15 2017, 10:26 AM
christof requested changes to D33586: ARMv8.1 support for LLVM AArch64.

Sorry, I was too quick. That blacklist is not working properly. Also, it would be good to add some test that show the blacklisting in operation. I just tried an example:

define void @test_atomic_load_add_i32(i32 %offset) nounwind {
     %old = atomicrmw add i32* @var32, i32 %offset seq_cst
   ret void
}

Which should not use WZR, but still uses it with the latest patch.

Jun 15 2017, 3:45 AM
christof accepted D33586: ARMv8.1 support for LLVM AArch64.

Looks good to me.
If you could still add a comment in the source on why you blacklist the instruction in the DeadRegisterDefinitionPass that would be appreciated.
Thanks for the nice patch. Looking forward to the patterns for the more relaxed memory models.

Jun 15 2017, 2:37 AM

Jun 14 2017

christof accepted D33773: [ARM] llc -arm-execute-only with floating point runs into UNREACHABLE.

LGTM

Jun 14 2017, 5:29 AM
christof added a comment to D33773: [ARM] llc -arm-execute-only with floating point runs into UNREACHABLE.

Actually I was inspired by getPICLabel in ARMAsmPrinter.cpp but I can change it to 'CP' if it makes more sense.

Yes please. Something that makes the connection with a constant pool clear would be helpful for people to make that connection later on.

Jun 14 2017, 3:40 AM
christof added a comment to D33773: [ARM] llc -arm-execute-only with floating point runs into UNREACHABLE.

I was about to approve it but, I have 1 little nitpick that I almost missed. Sorry for the dribble feed. It's about the naming of the global.

Jun 14 2017, 2:40 AM

Jun 13 2017

christof added a comment to D33773: [ARM] llc -arm-execute-only with floating point runs into UNREACHABLE.

The execute-only and PIC features cannot be enabled together. You probably should leave that unreachable in lib/Target/ARM/ARMAsmPrinter.cpp in place. You might even add one under the case ARM::CONSTPOOL_ENTRY in that file for good measure.

Jun 13 2017, 6:07 AM

Jun 9 2017

christof added a comment to D33773: [ARM] llc -arm-execute-only with floating point runs into UNREACHABLE.

This patch is definitely not the right approach. The LEA pseudo instruction should never be selected. I had a bit of a closer look and it seems that (ARMWrapper tconstpool) has various patterns that all match to PC relative pseudo instructions. They all should not happen in XO, so maybe this work is a bit more involved then expected. Also, we still have the ConstantIslandPass that is going to assume these constants pools are present in text and will split basic blocks for no good reason.

Jun 9 2017, 7:20 AM

Jun 8 2017

christof added a comment to D31709: [NFC] Refactor DiagnosticRenderer to use FullSourceLoc.

@rnk can you let me know if you are happy with the changes?
@rovka I assume the rebase did not change your mind about the patch. But please let me know if you have new reservations.

Jun 8 2017, 9:46 AM
christof updated the diff for D31709: [NFC] Refactor DiagnosticRenderer to use FullSourceLoc.

Rebased onto current HEAD and addressed the comments from Reid.

Jun 8 2017, 9:36 AM
christof commandeered D31709: [NFC] Refactor DiagnosticRenderer to use FullSourceLoc.

This refactoring was ready to land some time ago, except for a few small details. It's a shame if we don't commit this refactoring, so with @sanwou01
his agreement, I'm taking over this patch.

Jun 8 2017, 9:07 AM
christof added a comment to D33773: [ARM] llc -arm-execute-only with floating point runs into UNREACHABLE.

This might be a bit more difficult then expected. The code generator assumes the constant pool is PC-rel addressable using ADR at the moment. This is not the case when the constant pool lives in a different section. I should have known, sorry.

Jun 8 2017, 5:57 AM
christof added a comment to D33586: ARMv8.1 support for LLVM AArch64.

Would prefer to land this patch before adding the weaker ordering, but ST(OP) is dependent on weaker ordering. In any case this should perform equal to, or better than current LDX/STX on hardware that supports it.

Jun 8 2017, 2:19 AM

Jun 7 2017

christof added a comment to D33586: ARMv8.1 support for LLVM AArch64.

Seems like a good start for 8.1-A atomics to me. There are some things that might be improved upon, like supporting weaker orderings. Do you plan on doing that work as well?
Just a few questions inline.

Jun 7 2017, 10:25 AM
christof added a comment to D33773: [ARM] llc -arm-execute-only with floating point runs into UNREACHABLE.

The ARM code generator emits constant pools in a custom way. I believe that is to get them in-line with the code as a constant-pool in .text. But I think that Eli Friedman is right that for this use case it is possible to use the standard LLVM way of emitting them.

Jun 7 2017, 5:16 AM

Jun 2 2017

christof added a comment to D33586: ARMv8.1 support for LLVM AArch64.

After further digging, I believe that WZR can't be used for any of the LD<OP> instructions. If you do, you are actually encoding ST<OP>.

That's pretty much true. "ST<OP> ..." is an alias for "LD<OP> [wx]zr" so you're allowed to write both and they mean the same thing, with one caveat: if the instruction has acquire semantics there is no ST<OP> alias for it. You can only write "ldaddal wzr, w1, [x0]" that way, not as "staddal w1, [x1]" for example.

Jun 2 2017, 5:19 AM

Jun 1 2017

christof added a comment to D33586: ARMv8.1 support for LLVM AArch64.

Understood, this gives us correctness, will also have to ensure deadregister catches ordering and allows wzr for release or weaker.

Jun 1 2017, 3:59 AM

May 31 2017

christof added a comment to D33586: ARMv8.1 support for LLVM AArch64.

One other thing you have to be careful of: if an instruction loads into xzr or wzr then it's actually an "STwhatever" operation and defined not to have acquire semantics regardless of its mnemonic. So you probably have to modify AArch64DeadRegisterDefinitionsPass.cpp to ignore these instructions.

Yeah, this was a major concern of mine, technically it's an invalid mnemonic with ladaddal x0, xz, p. One solution was to custom lower all the atomic_load_X where operand 0 wasn't used, and create new AArch64ISD's for them. I wasn't thrilled about this for the first pass (because it seems to work anyway), but for the final it's something to consider.

May 31 2017, 8:35 AM

Mar 14 2017

christof closed D30414: [ARM] Don't pass -arm-execute-only to cc1as.

This landed as https://reviews.llvm.org/rL296454 some time ago. Not sure why it was not auto-closed. Closing manually.

Mar 14 2017, 9:28 AM

Feb 28 2017

christof abandoned D30291: Handle section header flags redefinitions similar to GAS.

Summary from email conversation:

  • GAS redefinitions is considered bad style
  • Removing the pre-constructed .text/.bss/.data/... sections from MC is a possible better way, although Rafael did not come back if he likes that approach.
Feb 28 2017, 1:55 AM
christof committed rL296454: [ARM] Don't pass -arm-execute-only to cc1as.
[ARM] Don't pass -arm-execute-only to cc1as
Feb 28 2017, 1:21 AM

Feb 27 2017

christof updated the diff for D30414: [ARM] Don't pass -arm-execute-only to cc1as.
Feb 27 2017, 9:35 AM
christof created D30414: [ARM] Don't pass -arm-execute-only to cc1as.
Feb 27 2017, 9:32 AM

Feb 23 2017

christof added a comment to D30169: Print unknown section header flags has hex in assembly output.

To clarify. This patch fixes some cases where:

clang -S file.c -o - | clang -c - -o file.o

is not identical to:

clang -c file.c -o file.o

because the assembly output 'forgets' some of the flags in the output assembly.

Feb 23 2017, 9:29 AM
christof created D30291: Handle section header flags redefinitions similar to GAS.
Feb 23 2017, 2:23 AM

Feb 21 2017

christof updated the diff for D30169: Print unknown section header flags has hex in assembly output.
Feb 21 2017, 6:51 AM
christof added a reviewer for D30169: Print unknown section header flags has hex in assembly output: rafael.
Feb 21 2017, 5:27 AM

Feb 20 2017

christof created D30169: Print unknown section header flags has hex in assembly output.
Feb 20 2017, 7:37 AM

Feb 7 2017

christof committed rL294298: [ARM] Make RWPI use movw/movt when available.
[ARM] Make RWPI use movw/movt when available
Feb 7 2017, 5:18 AM
christof closed D29487: Make RWPI use movw/movt when available by committing rL294298: [ARM] Make RWPI use movw/movt when available.
Feb 7 2017, 5:18 AM

Feb 3 2017

christof updated the diff for D29487: Make RWPI use movw/movt when available.

Added tests for ARM and Thumb2 without MOVT support and addressed the inline comments.

Feb 3 2017, 8:21 AM
christof updated the summary of D29487: Make RWPI use movw/movt when available.
Feb 3 2017, 3:18 AM
christof added a reviewer for D29487: Make RWPI use movw/movt when available: rafael.
Feb 3 2017, 3:05 AM
christof created D29487: Make RWPI use movw/movt when available.
Feb 3 2017, 2:30 AM

Jan 25 2017

christof added a comment to D27986: Print numeric section flag for OS/processor specific bits.

Hi Phrakar. I think you mean that there are no processor or OS specific flags with textual representation. But yes, SHF_ARM_PURECODE is the first of its kind. Makes me wonder if we are better of not having a textual representation at all for the flag.

Jan 25 2017, 3:59 AM

Sep 23 2016

christof added a comment to D24651: [SCEV] Try harder to find UB for NSW/NUW instr.

Let me elaborate a bit more on the UB of infinite loops. There existing code lists 2 cases of infinite loops triggered by poison:

Sep 23 2016, 8:17 AM

Sep 19 2016

christof added a comment to D24651: [SCEV] Try harder to find UB for NSW/NUW instr.

Apologies in advance for this off-the-cuff comment with only having skimmed through your patch, let me know if you've already addressed this: infinite loops are not UB in general, they're UB only if the loop has no side effects (i.e. no volatile or atomic operations and no IO).

Sep 19 2016, 2:25 AM

Sep 16 2016

christof retitled D24651: [SCEV] Try harder to find UB for NSW/NUW instr from to [SCEV] Try harder to find UB for NSW/NUW instr.
Sep 16 2016, 3:09 AM

Jun 9 2016

christof added a comment to D21142: [PDB] Move PDB functions to a separate file..

Hi Rui.

Jun 9 2016, 5:06 AM

Jun 8 2016

christof accepted D21117: Specify target in lifetime-asan test..

I've tested it and it removes the issue I saw. The following error is gone:
clang-3.9: error: unsupported option '-fsanitize=address' for target 'aarch64-arm-none-eabi’

Jun 8 2016, 3:19 AM
christof abandoned D21082: Do not assume that -fsanitize=address is valid option in clang tests.
Jun 8 2016, 3:17 AM
christof added a comment to D21082: Do not assume that -fsanitize=address is valid option in clang tests.
In D21082#451393, @kubabrecka wrote:

This doesn’t make sense to me, Clang is able to produce ASanified code even when the compiler itself isn’t ASanified (that’s what LLVM_USE_SANITIZER does). Where exactly is this test failing?

Jun 8 2016, 3:17 AM

Jun 7 2016

christof retitled D21082: Do not assume that -fsanitize=address is valid option in clang tests from to Do not assume that -fsanitize=address is valid option in clang tests.
Jun 7 2016, 9:37 AM

Feb 15 2016

christof updated subscribers of D8705: ScheduleDAGInstrs::buildSchedGraph() handling of memory dependecies rewritten..
Feb 15 2016, 7:10 AM

Jan 29 2016

christof added a comment to D16644: [AArch64] Add the scheduling model for Exynos-M1.

Is this M1 model incomplete since CNTv8i8 is not modeled? Is the default model incomplete since COPY is not modeled?

Jan 29 2016, 8:56 AM
christof added a comment to D16644: [AArch64] Add the scheduling model for Exynos-M1.

Still, I'm not sure what to make of the output. Any pointers, please?

Jan 29 2016, 2:10 AM

Jan 28 2016

christof added a comment to D16644: [AArch64] Add the scheduling model for Exynos-M1.

Just one comment: Last time I checked, if a sched model is missing from even a single instruction, the compiler will complain with incomplete sched model error when using the compiler (building the compiler was OK).

Jan 28 2016, 1:48 AM