- User Since
- May 22 2014, 1:24 PM (282 w, 5 d)
Sat, Oct 19
Fri, Oct 18
LGTM - see inline for a minor improvement.
The LangRef needs edits for this change.
I think this patch is also allowing the following constructs, but not testing for them.
Also worth mentioning: one of the suspects for the regression in PR33914 was a trailing cmov. That's gone now*, so we might want to implement the simpler fix (expand everything to 4) and re-check perf.
Can we make the x86 change to combineVectorSizedSetCCEquality() independently and before the change to TargetLowering?
Thu, Oct 17
Wed, Oct 16
LGTM with minor fixes as suggested.
Change implementation to a pure SLP bailout based on pattern-matching of a load-combine-reduction opportunity. Same test results.
Tue, Oct 15
I didn't step through the entire patch/tests, but the first few examples are flattened/simplified by -simplifycfg. Would it be better to look there to add the missing functionality? Is there a test in this patch that shows a case that simplifycfg can't already flatten?
I understand the direction, and I think there's general agreement on it (adding some more potential reviewers based on the bug report and llvm-dev thread), but:
- Add the tests in the patch description to a file in test/CodeGen/MSP430 as an NFC commit *before* applying this patch. Generate baseline test assertions for current asm using utils/update_llc_test_checks.py.
- Split this patch up. In the 1st step, we can add the TLI hook and 1-2 usages of that hook that show a test diff for MSP430. Ideally, we can add small tests for each proposed usage of the TLI hook.
Mon, Oct 14
Reverted at rL374851 - appears to cause a test-suite failure and stage 2 failure.
Rebased after committing baseline tests and cosmetic changes to the code.
Commandeering to rebase.
Added code comment about the inverted/commuted variations of these patterns.
Sun, Oct 13
Sat, Oct 12
@joanlluch - this is a case where we could transform incoming shift to select and benefit small targets as you've noted.
Fri, Oct 11
There seems to be general agreement that this is a temporary (stopgap) solution until we can do limited load combining in IR. So it's a question of whether we're ok with a cost model hack to overcome the motivating bugs while we figure out how to add a new pass.
Thu, Oct 10
Note that @reames suggested that we should publicize the previous patch to make sure there were no known unintended consequences from this change, so:
Wed, Oct 9
LGTM - I applied the patch locally and ran 'make check' and don't see failures now.
No diffs in this patch itself, but rebased after adding test at rL374190.
I don't have much experience with addrspaces or inbounds, so let me know if I should change anything else.
LGTM - you may want to annotate the title with 'NFC' when committing to indicate this patch doesn't actually change code.