Page MenuHomePhabricator

SjoerdMeijer (Sjoerd Meijer)
User

Projects

User does not belong to any projects.

User Details

User Since
Jan 26 2016, 2:17 AM (150 w, 2 d)

Recent Activity

Mon, Dec 3

SjoerdMeijer committed rL348110: [ARM] FP16: support vld1.16 for vector loads with post-increment.
[ARM] FP16: support vld1.16 for vector loads with post-increment
Mon, Dec 3, 12:29 AM
SjoerdMeijer closed D55112: [ARM] FP16: select vld1.16 for vector loads with post-increment.
Mon, Dec 3, 12:29 AM

Fri, Nov 30

SjoerdMeijer created D55112: [ARM] FP16: select vld1.16 for vector loads with post-increment.
Fri, Nov 30, 2:57 AM
SjoerdMeijer added a comment to D55059: [ARM] FP16: constant initialised v4f16 and v8f16 vectors.

Thanks for taking a look!

Fri, Nov 30, 1:43 AM
SjoerdMeijer committed rL347965: [ARM] Don't expand sdiv when optimising for minsize.
[ARM] Don't expand sdiv when optimising for minsize
Fri, Nov 30, 12:17 AM
SjoerdMeijer closed D54546: [ARM] Don't expand sdiv when optimising for minsize.
Fri, Nov 30, 12:17 AM
SjoerdMeijer added a comment to D54546: [ARM] Don't expand sdiv when optimising for minsize.

Thanks!

Fri, Nov 30, 12:00 AM

Thu, Nov 29

SjoerdMeijer created D55059: [ARM] FP16: constant initialised v4f16 and v8f16 vectors.
Thu, Nov 29, 9:10 AM
SjoerdMeijer added a comment to D54546: [ARM] Don't expand sdiv when optimising for minsize.

friendly ping

Thu, Nov 29, 9:00 AM

Tue, Nov 27

SjoerdMeijer updated the diff for D54546: [ARM] Don't expand sdiv when optimising for minsize.

Bail earlier if minsize or hwdiv is not enabled.

Tue, Nov 27, 7:03 AM
SjoerdMeijer added a comment to D54769: [FileCheck] New option -warn.

Ok, nice, looks like we actually agree: we want this to be errors, not warnings. Perhaps I conflated things too, and this was likely caused for practical reasons: how do we get all tests error free so that we the we can enable the error checking?

Tue, Nov 27, 1:50 AM

Mon, Nov 26

SjoerdMeijer added a comment to D54769: [FileCheck] New option -warn.

And I do remember a possibly valid use case for emitting a diagnostic. I've seen quite a few tests with names like test-pr12345.ll, that were reproducers of compiler crashes, but only contained CHECK-LABELS. I called them dubious tests before (but we don't want to use that term), because these tests are happy when a functions compiles, but don't tests anything whereas they probably should. There are probably a few exceptions where only CHECK-LABELs are okay, so I think a warning along the lines of "you're only checking labels, is that what you really want?" should be fair?

Mon, Nov 26, 4:51 PM
SjoerdMeijer added a comment to D54769: [FileCheck] New option -warn.

So, I'm not convinced that we can have a diagnostic for your case 3 that would really be meaningful and useful.

Mon, Nov 26, 4:21 PM
SjoerdMeijer added a comment to D54769: [FileCheck] New option -warn.

Looks like we are unhappy with FileCheck for many and different reasons:

  1. we want to error for overlapping dag checks,
  2. we want to error for the example in this diff: 1 but not all prefixes are checked,
  3. we want a diagnostic for my motivating example in D53710: only labels are checked (check-prefixes are renamed/refactored/misspelled, but not the tags in tests accordingly)
  4. CHECK-LABEL is confusing and probably mostly misunderstood
Mon, Nov 26, 3:55 PM
SjoerdMeijer added a comment to D54769: [FileCheck] New option -warn.

Sounds like that could be an error, rather than a warning, perhaps?

Mon, Nov 26, 1:00 PM

Fri, Nov 23

SjoerdMeijer updated the diff for D54546: [ARM] Don't expand sdiv when optimising for minsize.
  • Don't create div libcalls in ARM mode,
  • Bail for SREM instructions. This wasn't checked, and resulted in a crash. Thanks to @dmgreen for pointing this out. Will follow up on this.
Fri, Nov 23, 7:17 AM
SjoerdMeijer added a comment to D54769: [FileCheck] New option -warn.

fyi: as a first exercise, I found some (real) issues and fixed/committed them in https://reviews.llvm.org/rL347487

Fri, Nov 23, 3:09 AM
SjoerdMeijer committed rL347487: [ARM][NFC] codegen tests cleanup: remove dangling check prefixes.
[ARM][NFC] codegen tests cleanup: remove dangling check prefixes
Fri, Nov 23, 2:11 AM
SjoerdMeijer closed D54842: [ARM][NFC] codegen tests cleanup: remove dangling check prefixes.
Fri, Nov 23, 2:11 AM
SjoerdMeijer accepted D54790: [ARM] Prevent parallel macs for unsigned values.

LGTM

Fri, Nov 23, 2:05 AM
SjoerdMeijer created D54842: [ARM][NFC] codegen tests cleanup: remove dangling check prefixes.
Fri, Nov 23, 1:16 AM

Thu, Nov 22

SjoerdMeijer added inline comments to D54546: [ARM] Don't expand sdiv when optimising for minsize.
Thu, Nov 22, 8:41 AM
SjoerdMeijer updated the diff for D54769: [FileCheck] New option -warn.

I decided to continue here, and implemented the diagnostic that Paul suggested in the other ticket, because it's small and a nice demonstrator for this new option:

Thu, Nov 22, 6:00 AM

Wed, Nov 21

SjoerdMeijer updated the diff for D54769: [FileCheck] New option -warn.

This is small enough that I'd prefer to see it incorporated into the patch with the "real" warning, rather than have this temporary placeholder diagnostic.

Wed, Nov 21, 8:47 AM
SjoerdMeijer updated the diff for D54769: [FileCheck] New option -warn.

Is the purpose of this warning purely for testing your -warn implementation?

Wed, Nov 21, 8:08 AM
SjoerdMeijer updated the diff for D54546: [ARM] Don't expand sdiv when optimising for minsize.

Thanks for reviewing!

  • removed the unnecessary assert
  • added support for ARM mode, and tests.
Wed, Nov 21, 7:38 AM
SjoerdMeijer added inline comments to D54546: [ARM] Don't expand sdiv when optimising for minsize.
Wed, Nov 21, 6:03 AM
SjoerdMeijer updated the diff for D54769: [FileCheck] New option -warn.

Thanks for taking a look!

Wed, Nov 21, 1:31 AM

Tue, Nov 20

SjoerdMeijer created D54769: [FileCheck] New option -warn.
Tue, Nov 20, 12:36 PM
SjoerdMeijer added a comment to D53710: [FileCheck] Warn if a prefix is only used in LABEL checks.

I'm back :-) Catching up on the last messages.... I will post a patch for review soon that adds an option -warn to FileCheck, and will do some initial plumbing for that. If I understood correctly, it looks like we think that is best. After that initial plumbing, I will then return to this patch. Is that a plan?

Tue, Nov 20, 5:38 AM
SjoerdMeijer updated the diff for D54546: [ARM] Don't expand sdiv when optimising for minsize.

While looking what X86 as doing (and why it was doing the right thing), I noticed that I should just implement TargetLowering::BuildSDIVPow2. So that's what I am doing now, and thus it's no longer a DAGCombine change but just an ARM one.
Different tests and targets have been added, and a check for larger constants.

Tue, Nov 20, 4:58 AM

Mon, Nov 19

SjoerdMeijer accepted D54515: [ARM] Replace trunc for switch as a sink.

Thanks, looks okay to me.

Mon, Nov 19, 2:50 AM

Fri, Nov 16

SjoerdMeijer added a comment to D54546: [ARM] Don't expand sdiv when optimising for minsize.

A little bit of progress:

Fri, Nov 16, 9:37 AM

Wed, Nov 14

SjoerdMeijer retitled D54546: [ARM] Don't expand sdiv when optimising for minsize from [ARM] Don't expand sdiv to [DAGCombiner] Don't expand sdiv when optimising for minsize.
Wed, Nov 14, 3:35 PM
SjoerdMeijer added a comment to D54546: [ARM] Don't expand sdiv when optimising for minsize.

Thanks for commenting!
Yes, this should save 2 bytes.
I will check tomorrow:

  • about the SDIV legality, and where exactly the SDIV is expanded when it is not supported in HW. I see that sdiv is expanded very early if it is not available, immediately after the initial SDAG, and this code in visitSDIVLike does not get triggered. But again, will double check where exactly the magic happens. I've noticed that at least some tests are in place for other targets: the minsize check is no, the !VT.isVector() is required for some X86 and AArch64 tests, and without it the (knock on) effect is that the vector operations get scalarized and these tests fail, but I don't think this is related the legality of SDIV.
  • I missed this: "On Thumbv8, if you're dividing by a power of two greater than 128, I think you save zero byte", so will address that.
  • what exactly the benefit is on some other targets.
Wed, Nov 14, 3:30 PM
SjoerdMeijer created D54546: [ARM] Don't expand sdiv when optimising for minsize.
Wed, Nov 14, 2:33 PM
SjoerdMeijer added inline comments to D54515: [ARM] Replace trunc for switch as a sink.
Wed, Nov 14, 6:46 AM
SjoerdMeijer added a comment to D54515: [ARM] Replace trunc for switch as a sink.

Just a very quick first pass....some nitpicks:

Wed, Nov 14, 1:21 AM

Tue, Nov 13

SjoerdMeijer added a comment to D53710: [FileCheck] Warn if a prefix is only used in LABEL checks.

Sorry, just a quick message: I'm a few days out of office, and will get back to this end of this week.

Tue, Nov 13, 3:24 PM

Nov 9 2018

SjoerdMeijer added a comment to D53710: [FileCheck] Warn if a prefix is only used in LABEL checks.

Agreed with all previous comments! And yes, I messed up my last lit test example :-( and I looked at yours and Paul's, and agree. But anyway, we have a plan...

Nov 9 2018, 7:25 AM
SjoerdMeijer accepted D54308: [ARM] Don't promote i1 types in ARM CGP.

Looks like a straightforward fix to me.

Nov 9 2018, 6:37 AM
SjoerdMeijer accepted D54108: [ARM] Enable mixed types in ARM CGP.

LGTM

Nov 9 2018, 1:16 AM
SjoerdMeijer accepted D54254: [ARM] Reorganise some functionality in ARMParallelDSP.

LGTM

Nov 9 2018, 12:11 AM

Nov 8 2018

SjoerdMeijer added a comment to D53710: [FileCheck] Warn if a prefix is only used in LABEL checks.

Hi Paul and Joel, thanks for your help here, appreciate it! I thought this was
a nice little addition, low-hanging fruit, but it looks like it turns into
proper project! :-) Also from our discussions, it looks like we have not just
one but a few problems. If I try to summarise them:

Nov 8 2018, 3:25 AM

Nov 7 2018

SjoerdMeijer accepted D54192: [ARM] Fix CPSR liveness in tMOVCCr_pseudo lowering..

Looks very reasonable to me.

Nov 7 2018, 5:22 AM
SjoerdMeijer added a comment to D53710: [FileCheck] Warn if a prefix is only used in LABEL checks.

AMDGPU does this a lot intentionally for a shared label prefix and unique per run like ones for different subtargets

Nov 7 2018, 2:49 AM

Nov 6 2018

SjoerdMeijer added a comment to D53710: [FileCheck] Warn if a prefix is only used in LABEL checks.

Hi Joel, thanks for taking another look!

Nov 6 2018, 8:16 AM
SjoerdMeijer added a comment to D53710: [FileCheck] Warn if a prefix is only used in LABEL checks.

Friendly ping.

Nov 6 2018, 5:43 AM
SjoerdMeijer added inline comments to D54108: [ARM] Enable mixed types in ARM CGP.
Nov 6 2018, 1:32 AM

Nov 5 2018

SjoerdMeijer accepted D54094: [ARM] Turn assert into condition in ARMCGP.

Looks like a straightforward fix/addition to me.

Nov 5 2018, 3:04 AM
SjoerdMeijer accepted D54032: [ARM][ARMCGP] Remove unecessary zexts and truncs.

LGTM

Nov 5 2018, 2:41 AM

Nov 1 2018

SjoerdMeijer accepted D53972: [ARM][CGP] Negative constant operand handling.

Cheers, LGTM!

Nov 1 2018, 7:14 AM
SjoerdMeijer added a comment to D53972: [ARM][CGP] Negative constant operand handling.

Had a first look, mostly nits.

Nov 1 2018, 2:48 AM

Oct 31 2018

SjoerdMeijer added a comment to D49671: [SchedModel] Propagate read advance cycles to implicit operands outside instruction descriptor.

Apologies for being late to the party, but I am now looking into this too because we've seen some significant regressions with this change committed.
I am not blaming this commit, not yet, because I haven't fully understood the problem yet. As I am new to this area, it wanted to dump some initial thoughts here (because it takes me some time to get up to speed), perhaps people can comment.

Oct 31 2018, 6:20 AM

Oct 29 2018

SjoerdMeijer updated the diff for D53710: [FileCheck] Warn if a prefix is only used in LABEL checks.
  • Instead of a Dk_Note, it is a DK_Warning now.
  • Added test cases; hopefully demonstrating that when we have a prefix list, and one of them is used, we don't issue the warning.
Oct 29 2018, 3:31 AM
SjoerdMeijer committed rL345491: [ARM][NFC] Fix test inlineasm-X-allocation.ll.
[ARM][NFC] Fix test inlineasm-X-allocation.ll
Oct 29 2018, 1:49 AM
SjoerdMeijer closed D53748: [ARM] Fix test inlineasm-X-allocation.ll.
Oct 29 2018, 1:49 AM

Oct 26 2018

SjoerdMeijer committed rL345386: [ARM] Fix ARMCodeGenPrepare test cases.
[ARM] Fix ARMCodeGenPrepare test cases
Oct 26 2018, 7:23 AM
SjoerdMeijer closed D53746: [ARM] Fix ARMCodeGenPrepare test cases.
Oct 26 2018, 7:23 AM
SjoerdMeijer updated the diff for D53748: [ARM] Fix test inlineasm-X-allocation.ll.

This time with the correct diff attached; the previous one had the test cases swapped.

Oct 26 2018, 6:57 AM
SjoerdMeijer updated the diff for D53748: [ARM] Fix test inlineasm-X-allocation.ll.

Ah, I finally got it, and many thanks for explaining! I know see too that the mnemonic is totally irrelevant here (I got hugely distracted by vadd.f32).
I've learned something today! :-)

Oct 26 2018, 6:52 AM
SjoerdMeijer added a comment to D53748: [ARM] Fix test inlineasm-X-allocation.ll.

However, it seems wrong for the compiler to assign s0 as an operand without having FP registers available.

Oct 26 2018, 4:18 AM
SjoerdMeijer added a comment to D53710: [FileCheck] Warn if a prefix is only used in LABEL checks.

Thanks for looking at this! Answering this one first:

Oct 26 2018, 2:14 AM
SjoerdMeijer created D53748: [ARM] Fix test inlineasm-X-allocation.ll.
Oct 26 2018, 2:08 AM
SjoerdMeijer created D53746: [ARM] Fix ARMCodeGenPrepare test cases.
Oct 26 2018, 1:18 AM

Oct 25 2018

SjoerdMeijer added a comment to D53710: [FileCheck] Warn if a prefix is only used in LABEL checks.

I probably need to add a regression test for this, but wanted to check this idea with you first.

Oct 25 2018, 9:06 AM
SjoerdMeijer created D53710: [FileCheck] Warn if a prefix is only used in LABEL checks.
Oct 25 2018, 9:05 AM
SjoerdMeijer accepted D53632: [AArch64] Implement FP16FML intrinsics.

Looks OK to me. Thanks

Oct 25 2018, 12:19 AM

Oct 17 2018

SjoerdMeijer accepted D51983: [ARM] bottom-top mul support in ARMParallelDSP.

cheers, LGTM

Oct 17 2018, 4:58 AM
SjoerdMeijer committed rL344683: [ARM] Do not fuse VADD and VMUL, continued (2/2).
[ARM] Do not fuse VADD and VMUL, continued (2/2)
Oct 17 2018, 3:07 AM
SjoerdMeijer closed D53315: [ARM] Do not fuse VADD and VMUL, continued (2/2).
Oct 17 2018, 3:07 AM
SjoerdMeijer added inline comments to D51983: [ARM] bottom-top mul support in ARMParallelDSP.
Oct 17 2018, 2:53 AM
SjoerdMeijer committed rL344677: [ARM] Follow up of rL344671, attempt to pacify a buildbot.
[ARM] Follow up of rL344671, attempt to pacify a buildbot
Oct 17 2018, 12:53 AM
SjoerdMeijer committed rL344671: [ARM][NFCI] Do not fuse VADD and VMUL, continued (1/2).
[ARM][NFCI] Do not fuse VADD and VMUL, continued (1/2)
Oct 17 2018, 12:28 AM
SjoerdMeijer closed D53314: [ARM][NFCI] Do not fuse VADD and VMUL, continued (1/2).
Oct 17 2018, 12:28 AM

Oct 16 2018

SjoerdMeijer added a comment to D53315: [ARM] Do not fuse VADD and VMUL, continued (2/2).

With one bonus question, are the fused operations fast on the M7..?

Oct 16 2018, 8:45 AM
SjoerdMeijer added a comment to D35035: [InstCombine] Prevent memcpy generation for small data size.

Oops, sorry, forgot to reply, and also got distracted by a few other things. I ran one bigger benchmark, and didn't see anything worth mentioning. A first preliminary conclusion: looks like we don't miss much by not doing this lowering here. Disclaimer: I've tested only on Arm targets, definitely not on all interesting architecture combinations, and a handful of benchmarks.

Oct 16 2018, 8:16 AM
SjoerdMeijer updated the diff for D53315: [ARM] Do not fuse VADD and VMUL, continued (2/2).

Would it be worth adding a test for the M7 though? We seem to be a little lacking in our m-class FP tests.

Oct 16 2018, 8:02 AM
SjoerdMeijer updated the diff for D53315: [ARM] Do not fuse VADD and VMUL, continued (2/2).

should the f64 version not also be tested?

Oct 16 2018, 6:49 AM
SjoerdMeijer updated the diff for D53314: [ARM][NFCI] Do not fuse VADD and VMUL, continued (1/2).

I think moving VFP4 check into the useFPVMLx method would help make this easier to read.

Oct 16 2018, 5:45 AM
SjoerdMeijer updated the summary of D53314: [ARM][NFCI] Do not fuse VADD and VMUL, continued (1/2).
Oct 16 2018, 2:01 AM
SjoerdMeijer retitled D53315: [ARM] Do not fuse VADD and VMUL, continued (2/2) from ARM] Do not fuse VADD and VMUL, continued (2/2) to [ARM] Do not fuse VADD and VMUL, continued (2/2).
Oct 16 2018, 2:01 AM
SjoerdMeijer created D53315: [ARM] Do not fuse VADD and VMUL, continued (2/2).
Oct 16 2018, 1:59 AM
SjoerdMeijer updated the summary of D53314: [ARM][NFCI] Do not fuse VADD and VMUL, continued (1/2).
Oct 16 2018, 1:56 AM
SjoerdMeijer created D53314: [ARM][NFCI] Do not fuse VADD and VMUL, continued (1/2).
Oct 16 2018, 1:56 AM

Oct 12 2018

SjoerdMeijer accepted D53067: [AArch64] Swap comparison operands if that enables some folding..

Looks fine to me

Oct 12 2018, 6:03 AM

Oct 11 2018

SjoerdMeijer added inline comments to D53067: [AArch64] Swap comparison operands if that enables some folding..
Oct 11 2018, 5:40 AM
SjoerdMeijer added inline comments to D53067: [AArch64] Swap comparison operands if that enables some folding..
Oct 11 2018, 12:29 AM

Oct 4 2018

SjoerdMeijer added a comment to D35035: [InstCombine] Prevent memcpy generation for small data size.

I am doing some experiments with a hack that simply comments out the call to InstCombiner::SimplifyAnyMemTransfer. I've ran 3 smaller benchmarks. 2 didn't show any difference. The 3rd shows 1 tiny regression and 1 tiny improvement in a test case, and will probably not even show a difference in the geomean. I am doing this as a background task; tomorrow I will ran bigger benchmarks, on different platforms. But I guess we need some numbers for non-Arm platforms too.

Oct 4 2018, 8:33 AM
SjoerdMeijer committed rC343758: [AArch64][ARM] Context sensitive meaning of crypto.
[AArch64][ARM] Context sensitive meaning of crypto
Oct 4 2018, 12:41 AM
SjoerdMeijer committed rL343758: [AArch64][ARM] Context sensitive meaning of crypto.
[AArch64][ARM] Context sensitive meaning of crypto
Oct 4 2018, 12:40 AM
SjoerdMeijer closed D50179: [AArch64][ARM] Context sensitive meaning of option "crypto".
Oct 4 2018, 12:40 AM
SjoerdMeijer added a comment to D50179: [AArch64][ARM] Context sensitive meaning of option "crypto".

Thanks!

Oct 4 2018, 12:39 AM

Oct 2 2018

SjoerdMeijer added a comment to D35035: [InstCombine] Prevent memcpy generation for small data size.

The ultimate goal would be to simply always canonicalize to memcpy and not expand it ever in instcombine as mentioned in D52081.

Oct 2 2018, 10:46 AM
SjoerdMeijer added a comment to D50179: [AArch64][ARM] Context sensitive meaning of option "crypto".

@efriedma : apologies for the ping, but does this look reasonable?

Oct 2 2018, 12:54 AM
SjoerdMeijer accepted D52486: [AArch64][v8.5A] Add MTE as an optional AArch64 extension.

Looks okay to me.

Oct 2 2018, 12:53 AM

Sep 26 2018

SjoerdMeijer added inline comments to D52486: [AArch64][v8.5A] Add MTE as an optional AArch64 extension.
Sep 26 2018, 3:33 AM
SjoerdMeijer accepted D52463: [ARM] Fix for PR39060.

Thanks for clarifying! Looks good, with only a nit.

Sep 26 2018, 2:47 AM
SjoerdMeijer accepted D52488: [AArch64][v8.5A] Add Memory Tagging system registers.

LGTM

Sep 26 2018, 12:52 AM
SjoerdMeijer accepted D52493: [AArch64][v8.5A] Test clang option for the Memory Tagging Extension.

LGTM

Sep 26 2018, 12:51 AM