- User Since
- Dec 17 2012, 10:03 AM (257 w, 3 d)
Fri, Nov 17
Still LGTM :).
It makes sense to have it behind a hook for now, but could you follow-up with the targets owners so that it gets to be the default and we can get rid of the hook.
I believe this is general goodness and that we should use it for all targets going forward.
Wed, Nov 15
Mon, Nov 13
LGTM. Nitpicks below.
Thu, Nov 2
I don't know enough about shrink wrapping to know if that is feasible or not. It doesn't seem practical to me, but maybe Francis or Quentin can comment?
We'd actually want to do something relate to solve https://bugs.llvm.org/show_bug.cgi?id=33868.
We didn't have a chance to look at it yet.
Oct 24 2017
Move back the sorting of the Rules in main function.
This is not part of the optimization process.
One thing to note: When you try a group, there is no way out. I.e., the assumption is that groups are mutually independent. This is actually why I haven't push for more nesting level, because the patch doesn't provide a way to check that this is true.
For this, I was thinking there would be two kinds of partitioner. Those that deal with structure (number of operands, opcodes?*, nested instructions, etc.) and those that deal with predicates.
Oct 23 2017
Sounds good to me.
Oct 17 2017
For the record, what the grouping does is pretty much flattening the predicate class hierarchy. Long term, we may want to have a simpler class hierarchy.
Also, I have tried to same on memory comsumption and speed of tablegen, in particular, we duplicate operand indices and instruction identifier all over the place to make the checking of predicate correct. (isIdentical)
Oct 16 2017
Oct 13 2017
Oct 12 2017
LGTM with the previous nitpicks fixed.
Looks good modulo what I said in https://reviews.llvm.org/D37457 regarding GIM_CheckPointerToAny
Looks fine with one comment:
Do you think we need a new opcode in the state machine for that?
The added predicate part looks fine, but I didn't get why we need to change CodeGenInstruction into record just yet.
Looks mostly good to me.
Looks mostly good.
Couple of comments below.
LGTM with nitpicks.
Oct 11 2017
The problem is fixed.
Oct 10 2017
I'm a bit confused by the above. Should we allow legalization to look at other instructions, or should we not?
Oct 9 2017
LGTM for the generic part.
Oct 6 2017
Sorry Reid @rnk, I didn't know you were waiting for me. I saw already two people were saying it's good and I thought we didn't need a third one :).
LGTM with one comment: could we have the whole pattern being a helper function?
Oct 3 2017
I think we should probably just merge these.
Oct 2 2017
Sep 27 2017
This patch introduces an extra parameter to shouldCoalesce() (LiveIntervals*). Does this look acceptable?
Looks fine to me.
Looks sensible to me.
Sep 25 2017
Sep 21 2017
calculateSpillWeightAndHint, why is this not working here?
Sep 20 2017
Sep 19 2017
Sep 18 2017
For the record, playing with the order helps but to do the optimal (local) coloring here I would need to spend more time on the heuristic.
Again the easiest fix is probably the hints reconciling by region.
Sep 15 2017
LGTM with one suggestion
Sep 14 2017
Sep 13 2017
The AArch64 part looks good to me.
Sep 12 2017
Sep 11 2017
Sorry, that one slipped through!
Do you think that looking at the number of spilled live ranges is a good way to consider the trade-off for using hard hints, or do you perhaps have anything to add?
Sep 8 2017
The generic change looks fine though it sounds surprising that anyone would want that.
LGTM with a test case (.mir preferably) from PR34502
Aug 25 2017
Nitpicks below, no need to submit a new version.
Same comment as Krzysztof, would be nice if the debug printing was more intuitively discoverable (e.g., -debug alongside with -gen-register-info).
Aug 24 2017
Few more nitpicks, no need for more reviews.
I miss these test cases:
- Add a test case with def feeding into two different phis:
- In the same destination block
- In different basic block (one on the true branch, one on the false branch)
Aug 23 2017
Aug 22 2017
Good point. I think this is not needed. I'll update the other patch to use the std::next version. We can add this back at a later time if it's of value.
You can probably directly use setInsertPt with getFirstTerminator I think (i.e., no need to use std::next ;))
Note: Given the other review, I let you decide if we need that patch or not.
Just one nit.
I believe we use to do that by using std::next([Instr | Iterator]WeWantToInsertAfter), but I can see how that could make things easier, so LGTM