- User Since
- Apr 21 2016, 3:27 AM (65 w, 4 d)
Thu, Jul 20
Wed, Jul 19
LGTM with minor nits.
Tue, Jul 18
Mon, Jul 17
I think the commit message should elaborate a bit on why we want a single table.
Fri, Jul 14
Looks pretty trivial.
Hi, I tested this on our internal buildmaster and it seems to work. LGTM.
Hi, you could try looking into LLVMContext::emitError - it shouldn't be too hard to get hold of an LLVMContext, and that's what we use to report e.g. errors in inline asm in SelectionDAGBuilder.
Thu, Jul 13
I don't see anything wrong with this.
I'm not a huge fan of splitting string literals, since it makes it harder to grep for things, but I see there was already some precedent for that in the file and I assume this is what clang-format does too, so it's fine.
Wed, Jul 12
Thanks for the test-case and the explanations.
LGTM but you should probably wait for someone more versed in these things to give the final approval.
Tue, Jul 11
Mon, Jul 10
Fri, Jul 7
Actually, I do have one nit :)
Thu, Jul 6
Wed, Jul 5
Oops, I looked into it more closely and they're not timeouts, they're real issues:
Execution Context Diff:
/home/buildslave/buildslave/clang-native-arm-lnt/test/sandbox/build/tools/fpcmp: files differ without tolerance allowance
Hi, I just wanted to point out that this change also slows down some test-suite apps on ARM, as you can see here:
Wed, Jun 28
Tue, Jun 27
Jun 22 2017
Not for this patch, but I feel like some of the names and comments (e.g. emitCxxCaptureStmts) are starting to drift a bit from the implementation. Maybe at the end of this patch stack you could look into brushing them up?
Jun 19 2017
Jun 15 2017
Jun 13 2017
Jun 8 2017
Jun 7 2017
Jun 5 2017
Jun 2 2017
It certainly seems more restrictive than the previous patch, so I think it should be ok. @rengolin probably understands the subtleties here a bit better than I do.
May 30 2017
May 29 2017
May 24 2017
May 18 2017
May 17 2017
May 12 2017
May 11 2017
Removed the first loop and extracted a helper for adding renderers for default ops. I think this makes the body of the loop easier to read.
May 10 2017
May 9 2017
May 2 2017
Apr 28 2017
I just checked and the existing ARM tests fail without this patch because the available features aren't reset when entering the functions. They work with this patch + enabling the TableGen selector for ARM, so we'll have tests then :)
I think this patch is already doing a lot of things, I'd prefer committing the support for immediates separately and opening a PR for the test (and you can attach the X86 test to it so we can discuss with Igor). I need to double check this, but I think if I enable this for ARM (which I intend to do after this goes in) the existing ARM tests will already exercise this (for G_SDIV).
Apr 27 2017
Apr 26 2017
Thanks for splitting this out! The ARM part looks excellent :)
I think you could add some target-independent tests for this in unittests/CodeGen/GlobalISel/LegalizerInfoTest.cpp, to make sure it works to WidenScalar/MoreElements etc to a type that has a Libcall or Custom action. That way we won't have to depend on ARM for testing this functionality.
Thanks for all the explanations!
LGTM with that extra test, we can keep discussing the remaining points but I think it's ok to iterate in-tree. I'd really, really like to see the default change from 0 to 1 on RecomputePerFunction, it seems safer that way.
So, we have a new GISelAccessor with a new InstructionSelector for each unique subtarget (module + function attributes, as hashed in getSubtargetImpl). Why do we need to have module features and function features in each InstructionSelector? Isn't there a different InstructionSelector for each kind of function, and don't we magically get the right one when processing each function? What am I missing? It seems to me that we could get away with a single set of availableFeatures per InstructionSelector.
Apr 25 2017
I tried it and I can confirm that it works. Thanks!