- User Since
- Dec 17 2012, 10:03 AM (248 w, 6 d)
Thu, Sep 21
calculateSpillWeightAndHint, why is this not working here?
Wed, Sep 20
Tue, Sep 19
Mon, Sep 18
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.
Fri, Sep 15
LGTM with one suggestion
Thu, Sep 14
Wed, Sep 13
The AArch64 part looks good to me.
Tue, Sep 12
Mon, Sep 11
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?
Fri, Sep 8
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
Aug 21 2017
Aug 15 2017
Aug 14 2017
Based on this I think the solution should probably be kept in Greedy (+ possibly additional cleanup in the copy propagation pass).
LGTM with one nit
One nit below.
Thanks for your patience Geoff.
Aug 9 2017
Aug 8 2017
Aug 7 2017
The approach sounds sensible to me however, I'd like we use the AllocationPriority instead of the size of the register class.
Moreover, please refactor the code (I am thinking helper function) so that the definitions are processed in the same way.
Finally, keep the order of the operand as tie breaker.
Aug 4 2017
Aug 3 2017
Aug 2 2017
If we insert more converts we should raise Formula cost somehow (most likely leading to the formula drop).
Aug 1 2017
My guts say we should make the frame lowering logic more complex than it is already.
Long-term, it might be more straightforward to add implicit uses to return instructions, similar to the way we add implicit defs to calls, so we don't have to make return blocks a special case in LivePhysRegs. Not sure how hard that would be.
Jul 31 2017
Is that one flag that basically enables/disables all optimizations/folding ?
Yes, I would go with that for now. But I could see your concerns that we are going to generate crapy code for no good reason, thus you'd like more control.
Jul 28 2017
Disabled constant folding cast instructions as MIR currently doesn't infer legality of constants of type that it creates.
I think Hal would like to double check that.
Haha, never mind me, Hal already replied. (Sloppy tab!)
Jul 26 2017
- Move all the logic to handle the options in TargetPassConfig
- Refactor llc code to avoid code duplication, now only the run-pass pipeline is custom made
- Add MMI output parameter to be able to parse the module and populate the MachineModuleInfo after creating the pipeline
Jul 24 2017
The greedy allocator is already very complicated and I am not sure the additional complexity of the eviction track is worth it.
Is it something that could be cleaned up in machine copy propagation? The problem is very local so that sounds doable.
Jul 21 2017
Before removing all those assertions, could we talk that through first?
We started a conversion in D35730.
Jul 20 2017
BTW, my review does not include SystemZ changes ;)
Jul 14 2017
Jul 12 2017
Jul 11 2017
I see why second batch depends on this patch. I wonder if it should though.
Hence, do we rely guard that check with LSRWithInstrQueries?
Jul 10 2017