t.p.northover (Tim Northover)
Lord High Supreme Bullshitter

Projects

User does not belong to any projects.

User Details

User Since
Oct 18 2012, 4:53 AM (265 w, 4 d)

Recent Activity

Wed, Oct 25

t.p.northover accepted D30529: [RFC][GlobalISel] Enable legalizing non-power-of-2 sized types..

Thanks Kristof. I think this looks pretty reasonable as a starting point.

Wed, Oct 25, 6:27 AM

Sep 22 2017

t.p.northover accepted D36306: [ARM] Fix assembly and disassembly for VMRS/VMSR.
Thanks, this new one looks good too.
Sep 22 2017, 1:16 AM

Sep 18 2017

t.p.northover added a comment to D37968: [ARM] Fix for indexed dot product instruction descriptions.

Where is the restriction that only the lower 16 registers are allowed? Can you test quad regs, too? Just to make it clear the lane issue.

Sep 18 2017, 7:01 AM

Sep 5 2017

t.p.northover accepted D37493: Fix ARM bare metal driver to support atomics.

Oh hang on, I see you're undoing something which broke my assumptions. In which case, I completely agree with everything.

Sep 5 2017, 2:49 PM
t.p.northover added a comment to D37493: Fix ARM bare metal driver to support atomics.

I think this is wrong. "arm-none-eabi" covers RTOS threads too, where you really should use ldrex/strex loops unless you have good reason; and even kernels can be interrupted if they decide they want it.

Sep 5 2017, 2:48 PM

Aug 28 2017

t.p.northover accepted D36507: [ARM] GlobalISel: Select globals in PIC mode.

Looks reasonable to me.

Aug 28 2017, 7:45 AM
t.p.northover accepted D37030: Fix ARMv4 support.

I think the issues have been basically resolved here. The assertion can't hurt. LGTM!

Aug 28 2017, 7:35 AM

Aug 22 2017

t.p.northover accepted D37040: LowerAtomic: Don't skip optnone functions; atomic still need lowering (PR34020).

LGTM.

Aug 22 2017, 5:26 PM
t.p.northover added a comment to D37020: [AArch64][Falkor] Avoid generating STRQro* instructions.

There should be a test, but other than that only one nit (also with the comment).

Aug 22 2017, 1:47 PM

Aug 21 2017

t.p.northover added a comment to D36830: Use report_fatal_error for unsupported calling conventions.

I think it's a reasonable change. llvm_unreachable is clearly problematic.

Aug 21 2017, 1:55 PM

Aug 14 2017

t.p.northover accepted D36667: [ARM][AArch64] Cortex-A75 and Cortex-A55 support.

Looks reasonable to me. Looking forward to the RCPC instructions!

Aug 14 2017, 7:45 AM

Aug 10 2017

t.p.northover added a comment to D36575: [ARM] Assembler support for the ARMv8.2a dot product instructions.

Shouldn't the feature be predicated on HasV8_2aOps, not just NEON? It seems weird, from the tests, that we can target arm or thumbv7 but still have access to these instructions.

Aug 10 2017, 8:58 AM
t.p.northover added a comment to D36576: Fix -fPIC handling on arm64.

Also, make sure "llvm-commits" is a subscriber.

Aug 10 2017, 6:57 AM
t.p.northover updated subscribers of D36576: Fix -fPIC handling on arm64.
Aug 10 2017, 6:56 AM

Aug 9 2017

t.p.northover added a comment to D36522: [AArch64] Assembler support for v8.3 RCpc.

Looks good. Thanks.

Aug 9 2017, 7:59 AM
t.p.northover accepted D36515: [AArch64] Assembler support for the ARMv8.2a dot product instructions.

Looks reasonable to me.

Aug 9 2017, 7:45 AM
t.p.northover accepted D36522: [AArch64] Assembler support for v8.3 RCpc.

LGTM with one nit.

Aug 9 2017, 7:29 AM

Aug 8 2017

t.p.northover added a comment to D35817: Ban implicit _Complex to scalar conversions in C++.

What's going on with the OpenMP test changes?

Aug 8 2017, 12:13 PM
t.p.northover added a comment to D35817: Ban implicit _Complex to scalar conversions in C++.

Pingy.

Aug 8 2017, 11:34 AM

Aug 4 2017

t.p.northover accepted D36306: [ARM] Fix assembly and disassembly for VMRS/VMSR.

Looks reasonable to me, though please remember to add llvm-commits as a subscriber next time.

Aug 4 2017, 1:48 PM
t.p.northover added a comment to D36272: [CodeGen][x86_64] Enable 'force_align_arg_pointer' attribute at x86_64.

I've added "cfe-commits" as a subscriber since that's how we make sure the review request gets to the mailing list (which is still where review canonically takes place).

Aug 4 2017, 1:30 PM · Restricted Project
t.p.northover updated subscribers of D36272: [CodeGen][x86_64] Enable 'force_align_arg_pointer' attribute at x86_64.
Aug 4 2017, 1:29 PM · Restricted Project
t.p.northover accepted D35319: LSE Atomics reorg - Part I.

Thanks for going through so many revisions on this. I think it looks good now.

Aug 4 2017, 7:40 AM

Aug 2 2017

t.p.northover accepted D35883: [ARM] GlobalISel: Select simple G_GLOBAL_VALUE instructions.

Looks reasonable to me.

Aug 2 2017, 10:41 AM
t.p.northover accepted D36223: [AArch64] Simplify AES*Tied pseudo expansion (NFC)..

LGTM.

Aug 2 2017, 7:58 AM

Aug 1 2017

t.p.northover accepted D35919: [AArch64] Rewrite stack frame handling for win64 vararg functions.

Thanks Martin, I think this looks reasonable now too.

Aug 1 2017, 1:56 PM
t.p.northover accepted D36174: [AArch64] Fix a typo in isExtFreeImpl().

LGTM

Aug 1 2017, 1:39 PM
t.p.northover added inline comments to D35919: [AArch64] Rewrite stack frame handling for win64 vararg functions.
Aug 1 2017, 1:00 PM
t.p.northover updated the diff for D35817: Ban implicit _Complex to scalar conversions in C++.

Sounds like an improvement, I've updated the diff.

Aug 1 2017, 10:11 AM

Jul 31 2017

t.p.northover added a comment to D35970: Teach cc1as driver to accept ARM ropi/rwpi reloc model.

This looks like it'll get the build attributes wrong (since only PIC gets plumbed through to the backend). That's probably worse than simply rejecting the arguments.

Jul 31 2017, 3:56 PM
t.p.northover accepted D33435: [SelectionDAG] reset NewNodesMustHaveLegalTypes flag between basic blocks .

I'm really sorry this took so long. This patch is obviously an improvement. A PR against AArch64 would be sort of nice, but I don't think any more is needed because the actual chances of it affecting real codegen seems pretty small (significant vector code in the entry block?).

Jul 31 2017, 10:34 AM
t.p.northover added a comment to D35817: Ban implicit _Complex to scalar conversions in C++.

Ping.

Jul 31 2017, 10:00 AM
t.p.northover accepted D35737: [GSel]: Support Widening G_ICMP's destination operand..

Thanks. Looks reasonable to me.

Jul 31 2017, 9:27 AM

Jul 28 2017

t.p.northover added a comment to D35737: [GSel]: Support Widening G_ICMP's destination operand..

AArch64 has no 16-bit instructions (at least outside the SIMD unit). The first sensible type to legalize to is s32.

Jul 28 2017, 7:12 PM
t.p.northover accepted D35299: [AArch64] Tie source and destination operands for AESMC/AESIMC. .

Yep, I'd be happy with that too. Commit away!

Jul 28 2017, 1:59 PM
t.p.northover added a comment to D35299: [AArch64] Tie source and destination operands for AESMC/AESIMC. .

Not convinced by a Hungarian tagging like that. It's not what happens with existing pseudo-instructions. If anything they have a semi-descriptive name: "MOVaddr", "MOVaddrJT", "CMP_SWAP_8", ...

Jul 28 2017, 9:32 AM
t.p.northover added inline comments to D34873: Fix miscompiled 32bit binaries by mingw.
Jul 28 2017, 9:29 AM

Jul 27 2017

t.p.northover accepted D35927: [NFC] standardized suffixes for LSE Atomics mnemonics.

Looks fine. Thanks for splitting it out.

Jul 27 2017, 6:50 AM

Jul 26 2017

t.p.northover added a comment to D35919: [AArch64] Rewrite stack frame handling for win64 vararg functions.

There's far too much ad-hoc hackery going on here for my tastes. This is the 3rd (soon to be 4th) patch trying to get something working, I think it's time to step back and really look at the code that's being modified. Or at least come up with a thorough set of tests.

Jul 26 2017, 3:15 PM
t.p.northover added inline comments to D35319: LSE Atomics reorg - Part I.
Jul 26 2017, 11:28 AM
t.p.northover added a comment to D35319: LSE Atomics reorg - Part I.

Thanks very much for updating the patch. It's starting to look a lot more like how I'd expect these to be implemented.

Jul 26 2017, 11:11 AM

Jul 25 2017

t.p.northover added a comment to D30529: [RFC][GlobalISel] Enable legalizing non-power-of-2 sized types..

Hi Kristof,

Jul 25 2017, 3:58 PM
t.p.northover accepted D35540: [AArch64] Add a test for float argument passing to win64 vararg functions.

Looks fine.

Jul 25 2017, 7:18 AM
t.p.northover accepted D35543: [AArch64] Update a comment in a test.

Go for it.

Jul 25 2017, 7:18 AM

Jul 24 2017

t.p.northover updated the diff for D35817: Ban implicit _Complex to scalar conversions in C++.

Sorry, uploaded version with slightly wrong tests.

Jul 24 2017, 2:42 PM
t.p.northover created D35817: Ban implicit _Complex to scalar conversions in C++.
Jul 24 2017, 2:42 PM
t.p.northover accepted D35720: [AArch64] Reserve a 16 byte aligned amount of fixed stack for win64 varargs.

Thanks, this looks reasonable now I think.

Jul 24 2017, 2:01 PM

Jul 21 2017

t.p.northover added a comment to D35299: [AArch64] Tie source and destination operands for AESMC/AESIMC. .

I'm not entirely sure what you mean here.

Jul 21 2017, 1:44 PM
t.p.northover added inline comments to D35720: [AArch64] Reserve a 16 byte aligned amount of fixed stack for win64 varargs.
Jul 21 2017, 9:49 AM

Jul 20 2017

t.p.northover added a comment to D35299: [AArch64] Tie source and destination operands for AESMC/AESIMC. .

I'm afraid that it would create problems for the assembler too, wouldn't it?

Jul 20 2017, 3:15 PM
t.p.northover added a comment to D35701: Break up Targets.cpp into a header/impl pair per target type[NFCI].

I tended to keep function definitions in the class declaration unless it caused additional includes to be necessary.

Jul 20 2017, 2:27 PM

Jul 19 2017

t.p.northover added a comment to D35635: [ARM] Optimize {s,u}{add,sub}.with.overflow.

There doesn't seem to be any test of the inversion logic.

Jul 19 2017, 10:20 AM
t.p.northover accepted D35544: [AArch64] Force relocations for all ADRP instructions.

Looks fine to me other than a nit about the MachO test. Unless the fix to that gets weird and you want to confirm it's right you can commit without uploading another revision.

Jul 19 2017, 6:35 AM

Jul 18 2017

t.p.northover added a comment to D35544: [AArch64] Force relocations for all ADRP instructions.

So it seems to me like it actually does use ADRP, but probably via some other codepath, since it does generate a relocation. But fixing it for handwritten assembly obviously also is worthwhile.

Jul 18 2017, 3:28 PM
t.p.northover added a comment to D35544: [AArch64] Force relocations for all ADRP instructions.

I'd be interested in knowing why this isn't done by default (i.e. why this doesn't need to be done at all for MachO) btw, if someone has got time to explain.

Jul 18 2017, 7:39 AM

Jul 17 2017

t.p.northover accepted D34474: [AArch64] Extend CallingConv::X86_64_Win64 to AArch64 as well.

I think this looks pretty much as I'd expect. Thanks for working on the generic refactoring.

Jul 17 2017, 12:16 PM
t.p.northover accepted D35494: [ARM|CodeGen] Improve the code in FastISel.

Looks like a good change other than one nit. Feel free to commit with that fixed.

Jul 17 2017, 11:56 AM
t.p.northover accepted D35493: [AArch64] Use 16 bytes as preferred function alignment on Cortex-A73..

Looks fine to me.

Jul 17 2017, 10:12 AM

Jul 16 2017

t.p.northover added a comment to D35469: [AArch64] Make "Unkown Fixup" error msg more useful by giving Fixup Kind Name.
  1. Is a functional change since it switches an assertion to an actual error report. It would be better to use "Ctx.reportError", and since I'm assuming you have places where this makes a difference to you, tests would be useful if possible. (If it was only useful in debugging an already fixed problem and you now can't trigger them we'd probably let it squeeze by, but the lack of tests should be called out and justified).
  2. Trivial improvement, go ahead (you didn't need to post a patch for review for this kind of change). Shouldn't be part of the same commit as the others though.
  3. Generally not encouraged since it messes up "git blame" history.
Jul 16 2017, 7:04 PM

Jul 14 2017

t.p.northover added a comment to D34474: [AArch64] Extend CallingConv::X86_64_Win64 to AArch64 as well.

So, in unifying it and making it generic, what backwards compat requirements are there on the IR? Can I rename the calling convention enum (X86_64_Win64, to e.g. just Win64) and just force all callers (at least clang) to catch up at once?

Jul 14 2017, 10:43 AM
t.p.northover added inline comments to D35307: [AArch64] Initial SVE register definitions.
Jul 14 2017, 8:13 AM
t.p.northover added a comment to D34474: [AArch64] Extend CallingConv::X86_64_Win64 to AArch64 as well.

I agree with Reid. There are now 2 targets using this and it seems like a generic requirement of Windows & Wine interoperability; it's time to make it generic.

Jul 14 2017, 7:56 AM
t.p.northover added inline comments to D35209: [ARM] Unify handling of M-Class system registers.
Jul 14 2017, 7:50 AM

Jul 13 2017

t.p.northover added a comment to D35319: LSE Atomics reorg - Part I.

I implemented the memory ordering semantics for all the LSE Atomics with Intrinsics.

Jul 13 2017, 2:35 PM
t.p.northover added a comment to D35319: LSE Atomics reorg - Part I.

OK, that pass looks fine to me now. And we're still going to trust you know what you're doing with the scheduling. Which leaves the churn in the TableGen files...

Jul 13 2017, 12:26 PM
t.p.northover added inline comments to D35319: LSE Atomics reorg - Part I.
Jul 13 2017, 11:27 AM
t.p.northover added inline comments to D35319: LSE Atomics reorg - Part I.
Jul 13 2017, 8:43 AM
t.p.northover accepted D35357: [AArch64] Enable the mnemonic spell checker.

Looks reasonable to me.

Jul 13 2017, 8:20 AM
t.p.northover added inline comments to D35319: LSE Atomics reorg - Part I.
Jul 13 2017, 8:00 AM
t.p.northover added inline comments to D35319: LSE Atomics reorg - Part I.
Jul 13 2017, 7:53 AM
t.p.northover accepted D35239: [llvm-objdump] Properly print MachO aarch64 addend relocations.

Looks reasonable to me.

Jul 13 2017, 7:44 AM
t.p.northover added a comment to D35319: LSE Atomics reorg - Part I.

What does whatever mean, in this context?

Jul 13 2017, 7:34 AM
t.p.northover added a comment to D35319: LSE Atomics reorg - Part I.

This diff covers lots of different areas:

Jul 13 2017, 7:17 AM
t.p.northover accepted D35309: [AArch64] Add preliminary support for ARMv8.1 SUB/AND atomics.

This looks fine to me.

Jul 13 2017, 7:02 AM
t.p.northover accepted D35006: [AArch64] Implement support for windows style vararg functions.

Thanks for updating the patch. Looks really good to me, just one trivial comment about a comment.

Jul 13 2017, 4:55 AM

Jul 11 2017

t.p.northover accepted D35266: [AArch64] Remove unused IsDarwin & IsNotDarwin predicates (NFCI). .

Looks fine.

Jul 11 2017, 12:41 PM
t.p.northover added a comment to D35239: [llvm-objdump] Properly print MachO aarch64 addend relocations.

llvm-commits doesn't appear to be involved here.

Jul 11 2017, 7:25 AM

Jul 6 2017

t.p.northover added a comment to D35006: [AArch64] Implement support for windows style vararg functions.

OK, I now see that this actually works. You're essentially redefining the CFA (canonical frame address -- where LLVM bases its SP calculations) to the aligned SP after the extra GPRs have been pushed.

Jul 6 2017, 7:34 PM
t.p.northover added a comment to D35006: [AArch64] Implement support for windows style vararg functions.

I've thought some more and it seems pretty clear now that the difference is exactly that extra GPRSaveSize in FrameLowering. That needs to be incorporated into LLVM's view of the stack size and layout.

Jul 6 2017, 6:04 PM
t.p.northover added inline comments to D35006: [AArch64] Implement support for windows style vararg functions.
Jul 6 2017, 4:25 PM

Jun 30 2017

t.p.northover accepted D34861: [ARM] Fix cmpxchg at O0 for big-endian.

I think this looks reasonable. Thanks for looking into it.

Jun 30 2017, 11:54 AM
t.p.northover added a comment to D34875: ARM: Report error for invalid use of AAPCS_VFP calling convention.

I think that they do, because report_fatal_error() causes compilation to terminate so the subsequent tests don't generate a message which can be checked.

Jun 30 2017, 10:16 AM
t.p.northover added a comment to D34875: ARM: Report error for invalid use of AAPCS_VFP calling convention.

What's the use case here? For people using __attribute__((pcs(...))) Clang should be producing the diagnostic (and I'm not even entirely convinced it should be an error, if your code only contains integer types there's not actually any problem).

Jun 30 2017, 7:46 AM

Jun 29 2017

t.p.northover accepted D34837: [GISel]: New Opcode G_FLOG/G_FLOG2.

Looks fine to me.

Jun 29 2017, 4:02 PM
t.p.northover accepted D32529: [GlobalISel] Make multi-step legalization work..

Looks fine to me. Just one tiny nit (sorry).

Jun 29 2017, 12:15 PM
t.p.northover accepted D34813: [llvm-objdump] Handle invalid instruction gracefully on ARM.

Looks fine to me. Next step, enable the crystal ball for Thumb!

Jun 29 2017, 7:55 AM

Jun 28 2017

t.p.northover created D34779: Fix floating point promotions for overload purposes.
Jun 28 2017, 2:16 PM
t.p.northover accepted D34730: [AArch64] Make assert messages uniform [NFC].

Fair enough. It's certainly a reasonable change.

Jun 28 2017, 12:20 PM
t.p.northover accepted D34408: [ARM] - Add the option to directly access TLS pointer.

Thanks Strahinja, this looks good to me.

Jun 28 2017, 9:45 AM

Jun 27 2017

t.p.northover added a comment to D34730: [AArch64] Make assert messages uniform [NFC].

I'd be pretty tempted to remove them entirely. Those are the only 3 LLVM supports at all and assertions for (extremely) hypothetical future additions seem to be taking it a bit far.

Jun 27 2017, 8:36 PM
t.p.northover added a comment to D34474: [AArch64] Extend CallingConv::X86_64_Win64 to AArch64 as well.

I think MSVC has to have changed the ABI from AAPCS if they've managed to get a "void *" va_list to work; the extra complexity in AAPCS isn't there for the giggles. x86 appears to mirror floating args into integer registers.

Jun 27 2017, 3:10 PM
t.p.northover accepted D34710: [GISel]: New Opcode G_FEXP/G_FEXP2.

Looks fine to me.

Jun 27 2017, 1:45 PM
t.p.northover accepted D34698: [AArch64] Inline callee if its target-features are a subset of the caller.

Apologies, I misread the CHECK-LABEL line as the test by mistake. Looks good to me!

Jun 27 2017, 1:44 PM
t.p.northover added a comment to D34698: [AArch64] Inline callee if its target-features are a subset of the caller.

There don't appear to be any negative tests here (i.e. you could replace the function with "return true" and nothing would fail). Other than that, I think it's OK for the existing features. We're going to have to watch out in future though: +strict-align could easily have been implemented as +no-strict-align for example.

Jun 27 2017, 10:22 AM

Jun 20 2017

t.p.northover added a comment to D34409: Use 64bit jump table with large code model on 64bit.

For AArch64 there was a previous discussion here: https://reviews.llvm.org/D32564 (some replies don't seem to be here, the thread is at http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20170424/448545.html). That patch would supersede this one I believe, but until I get around to resurrecting it this patch at least makes things work.

Jun 20 2017, 2:40 PM
t.p.northover added a comment to D34409: Use 64bit jump table with large code model on 64bit.

The entirely sensible assumption of the PPC backend is that a single function is no longer than 2GB/4GB.

Jun 20 2017, 1:56 PM
t.p.northover accepted D34409: Use 64bit jump table with large code model on 64bit.

This looks reasonable to me.

Jun 20 2017, 12:24 PM
t.p.northover added a comment to D34408: [ARM] - Add the option to directly access TLS pointer.

I think this fits in better as a subtarget feature controlled by "-mattr". The implementation could then be almost all in TableGen, just using different patterns for ARMthread_pointer (giving greater symmetry with the existing path). It would also make it much neater to pipe through to clang.

Jun 20 2017, 9:43 AM
t.p.northover added a comment to D34401: [SelectionDAG] Fix UMULO fallback.

Of course. It should be committed as 305800.

Jun 20 2017, 8:02 AM
t.p.northover added a comment to D34402: [AArch64] Preserve register flags when promoting a load from store..

I'm not sure findMatchingStore can do it easily. During its reverse scan it doesn't know what the value register is going to be and keeping track of the most recent kill of all registers seems a little excessive.

Jun 20 2017, 7:48 AM