- User Since
- Aug 18 2016, 4:39 AM (52 w, 2 d)
LGTM, I think it's a nice simplification and was already discussed in D36899. Please check the formatting before committing.
Fri, Aug 18
Thanks for clarifying that! Sounds and looks good to me.
Please clarify the question about adding FeaturePostRAScheduler to some processors and maybe hold off a bit with committing, in case @rengolin or @
I think this change makes sense and will be convenient when we move ARM codegen to the MachineScheduler. I just have a quick question about adding FeaturePostRAScheduler to some processors.
Thu, Aug 17
Wed, Aug 16
I think this changes makes sense and thanks for getting rid of the if (that's not needed any longer) and making the debug message clearer too! I guess adding a test for the case where SecondSU has dependencies that are scheduled between FirstSU and SecondSU is difficult, as in most fused instruction pairs the second instruction only depends on the first instruction?
Thanks for all the feedback! I'll update the patch soon.
Mon, Aug 14
Thanks for your feedback Eli. I've update the patch to only use incremental updates if the size of a BB is over a certain threshold. I've chosen 500 instructions for now, but a slightly higher one would probably be OK too.
LGTM. It's not used at the moment and should be removed.
Sun, Aug 13
Sat, Aug 12
Removed M-class triple check.
Fri, Aug 11
Thu, Aug 10
Ping. Any thoughts?
Looks like a straightforward NFC that makes the code clearer. LGTM, but maybe someone else should have a quick look too.
Wed, Aug 9
LGTM, this looks like a NFC to me and range-based for loops make the code easier to read. Please check the loop I added a comment to before committing.
Tue, Aug 8
I think this change should still go in (in combination with D35627) to get proper error messages in the attribute case. Please let me know if there are any more concerns.
Mon, Aug 7
Sun, Aug 6
Sat, Aug 5
Fri, Aug 4
Besides the outstanding comments, LGTM
@MatzeB are you happy with the change too? It appears you have to remove your 'This revision now requires changes to proceed.' setting before Phab will mark this patch as accepted.
Thu, Aug 3
Wed, Aug 2
Thanks, I'm happy with the indentation and lib/Target/ARM/ARMISelDAGToDAG.cpp was a good spot.
LGTM, looks like a straight-forward improvement. I just have 2 nits. Please hold off with committing for a bit, so other people have time to voice concerns.
Using -start-before=livedebugvalues . I had to adjust the offset, but I don't think the exact offset is crucial for the test case. Maybe @aprantl can confirm that?
ping. @rengolin any more thoughts?
I'll commit D36151 after this one goes in, so we see if this change fixes the buildbot
Tue, Aug 1
Agreed, in hindsight the current solution is quite heavy-handed. 1 register
I just had a quick look and left a few comments. I'm just curious, what kind of testing did you do & did you run any benchmarks?
Yes, D36094 fixes the problem too! Here's what's going on in the test case, without any of the 2 patches:
I'm just curious, did you run any benchmarks with this change? I think it's quite clear that using the block frequencies is a good idea, but it would be awesome to know if/by how much it improves things :)
Mon, Jul 31
Updated to use LLVMContext::emitError instead of fatal error. Before committing D35569 needs to land, otherwise the second invocation of in the test will not produce errors.
Thanks for having a look. Committed as is because some print functions are used in implementations of
raw_ostream &operator<<(raw_ostream &OS, const X &X), which in turn are used in DEBUG()
Added more checks to test case, thanks @mcrosier for having a look! :)
Sat, Jul 29
Renamed pseudo instructions, will commit soon
Fri, Jul 28
How about something like AESIMCrrTied?
Thu, Jul 27
I think it shouldn't be to hard to fold the print() functions into the dump() functions in most cases. I could update the patch. The only benefit of having those print functions would be debugging, in the rare case someone wants to print to a different stream then stderr.
This patch also adds guards for a few functions not used by dump(), but only inside DEBUG(). I could split them off if that's preferred.