- User Since
- Apr 21 2016, 3:27 AM (91 w, 3 d)
Thu, Jan 18
Wed, Jan 17
Mon, Jan 15
LGTM, but I think you should commit the BOTO change separately.
LGTM, but I have a couple of nitpicks:
- Why is this PATCH 2/6? It doesn't seem related to either 1/6 or 3/6 and can be committed independently
- The summary just repeats the title. A better summary would be something along the lines of "Don't force the number of jobs on the ARM builders. This makes it possible to fall back on the number of jobs configured for the slave instead."
Fri, Jan 12
Wed, Jan 10
Thu, Jan 4
Dec 22 2017
Dec 20 2017
Dec 18 2017
Dec 11 2017
LGTM until we can handle big endian. Sorry about the delay.
Dec 4 2017
Dec 1 2017
This looks fine, but I think it would be better if the slave names weren't tied to what is currently running on them, but rather to the machine configurations. E.g. if there's no difference in the environment of linaro-d05-01-quick and linaro-d05-global-isel, they should just be linaro-d05-01 and linaro-d05-02. But for instance if the machines running the libcxx bots need something special, then they can be linaro-d05-libcxx-01 and linaro-d05-libcxx-02. That way, if we want to repurpose a slave to run a different builder, it can keep its name without looking awkward, and it's also easy for us to tell which machines have different environments. What do you think?
Nov 30 2017
Nov 29 2017
Nov 28 2017
I don't know if this fixes every edge case that we currently have with and/or immediates in DAGISel, but it works for the PKHBT and doesn't seem to break anything else, so in the interest of simplicity it's probably a good route to take. You should add a summary when you commit though.
Nov 23 2017
Nov 21 2017
Nov 20 2017
Nov 16 2017
Nov 15 2017
Nov 14 2017
Nov 13 2017
Hi, I've updated the ARM tests, and you'll need to also commit this together with your patch: https://reviews.llvm.org/differential/diff/122657/
Nov 10 2017
Nov 7 2017
Awesome, I could use something like this. LGTM with a few nits.
Sure, will do.
Nov 6 2017
Nov 3 2017
I think it would be simpler to add a RUN line to the existing test (you can rename it to just legalizer.mir if you think the current name would be misleading).
Nov 2 2017
Sorry about the indirect test, but it's a bit difficult to test directly since this is restricting functionality rather than adding it. Are we interested in having tests for failed imports? I haven't seen any, but I may have missed them...
Nov 1 2017
I tried to give this a run  on top of r317072 on an NVIDIA TK1 (Cortex-A15) and I'm getting some failures in the test-suite (in 42 of the benchmarks) along the lines of . I think we should fix those before worrying about performance numbers...
Oct 30 2017
Oct 26 2017
Oct 25 2017
LGTM with nit.
Oct 24 2017
Oct 19 2017
Hi, here's the patch for the ARM GlobalISel tests: https://reviews.llvm.org/differential/diff/119563/
Let me know if there's anything else that needs addressing on ARM.
Oct 18 2017
Oct 17 2017
Sorry, I just noticed this. I'll have a look at the ARM side of things.
This looks correct, but is there anything removing DestBB from the newly created block's successors if necessary?
Oct 12 2017
Add unit tests and update summary before commit.
Oct 11 2017
Update to take into account isDef only when ignoring virtual defs, since this is what the getHashValue function assumes.
First of all, this changes from using isPhysicalRegister to using !isVirtualRegisters, which also includes the sentinel values that we're interested in.
Secondly, it changes from checking MO.getReg to checking MO.isIdenticalTo, which takes into account MO.getReg, MO.getSubReg and MO.isDef. These are the same things that the hash function combines when getting the hashes.
This is a latent issue that triggers on one of the buildbots if the planets align. I'm holding one of Sanjay's InstCombine patches (r314698) hostage because of this bug, so please have a look. Thanks!
Oct 6 2017