- User Since
- Mar 27 2015, 6:20 AM (165 w, 1 d)
Thu, May 24
Wed, May 23
I am not an expert in encryption but how likely it is for 'key' to be zero ?
Other than that, LGTM
Sun, May 20
Thu, May 17
Tue, May 15
LGTM but then I wrote it originally so better to wait for comments from others.
Mon, May 14
Suggestion for commit message - "Add support and tests for contiguous Prefetch instructions different variants."
The problem i find sometimes with long title is that it it truncated.
Sun, May 13
Sat, May 12
Fri, May 11
Thu, May 10
Having discussed with Sander and Florian in detail I now agree it is better to add properly grouped Scheds later.
I would then suggest adding the following to AArch64Schedule.td and then simply assigning the appropriate Scheds.
Much less work and neater doing it now when the instruction implementation is being done.
I might be good to put commit-message/description.
Wed, May 9
Thu, May 3
Does FDIV use both the resources for 8 cycles? Writing [8,8] would imply that.
Wed, May 2
Ah! OK, I see. Thanks for the clarification.
Thanks for this check.
If ResourceCycles is not specified then it should default to 1 . I believe most people write with that interpretation.
If ResourceCycles is specified for at least one resource, it makes sense from correctness POV that it be specified for all (even though it might get verbose at time).
Tue, May 1
Hi Kristof. Thanks for this.
Mon, Apr 30
Apr 27 2018
Please rephrase this part of commit message as it is hard to read/understand - "And even if statistics is disable and code
related to it will be eliminated the invocation to getExact itself will not be eliminated because it may have side-effects like creation of new SCEVs".
AFAIU the correctness check would now get gated on STATS colelction and that may not be right. I would recommend separating the two.
Apr 26 2018
Apr 25 2018
Would removing 'isKnownViaInduction' have any impact of the number of cases 'analyzeable by scev?
Apr 24 2018
LGTM. Probably wait a day before committing in case Renato/others have a comment/suggestion.
Apr 23 2018
Could you please generate diff with -U999 to give more context to the patch. Thanks.
Apr 20 2018
Any test possible here?
AFAIU this change would affect other targets. Probably best to add more reviewers (owners for other targets).
Apr 19 2018
Thanks for this Sebastian. Overall, this definitely look good for vectorization.
Apr 18 2018
Its good you mention 'patch [4/4] in a series' in commit message. But could you please also link them up by providing link to previous reviews etc.
Might be easy for tracking later.
Apr 4 2018
For the test case, the change looks good, though I am surprised the current scheduling algorithm does that (i.e. scheduling store just one cycle after load which definitely would result in stalls, especially for in-order processors). I am guessing the latency it is picking up is 1?
Apr 3 2018
Mar 26 2018
Mar 25 2018
Mar 22 2018
Thanks for this.
Mar 21 2018
Thanks for this. If I am getting this right, just like CompleteModel ensures no instruction has a missing schedule, this patch will ensure no instruction has multiple schedule (instregex) assigned?
Mar 6 2018
Mar 5 2018
I assume no tests are impacted by this. Otherwise, looks LGTM
Feb 27 2018
By the way, did you do any bench-marking to see if there were any impact of these changes, although from the changes it looks to me there wont be anything significant, and besides I understand this is simply for CompleteModel.
Feb 16 2018
Jan 30 2018
Jan 18 2018
Jan 17 2018
Jan 2 2018
Dec 20 2017
Dec 4 2017
LGTM. Thanks for this.
Nov 28 2017
Nov 27 2017
Just a suggestion: To ease review, could this be split into smaller patches along the lines you mention - noop stores, partial overwrites, stores before frees/lifetime_ends ?
Nov 24 2017
Nov 23 2017
Full context (diff -U999) is missing. Also you may want to write a brief intro which you can use as commit message.
Nov 22 2017
Nov 21 2017
LGTM. Thanks for this.
Thanks for this.
Thanks for this. Is it possible to put a test? If not, perhaps you can share the output generated for a purposely failing case here (by omitting a referenced resource).
Nov 20 2017
Nov 16 2017
Nov 10 2017
Nov 9 2017
Nov 8 2017
This patch is now dependent on https://reviews.llvm.org/D39808
Nov 7 2017
As Oli mentioned, I did not find reference to PSR in ARM-ARM when preparing these tables. Leaving it to Renato to decide whether it should still be added.
If you do end up adding, and the encoding is same as XPSR, you could use the Mask bits 1,0,1 to differentiate against existing XPSR encoding entry (otherwise Searchable Table will complain about uniqueness).
Nov 6 2017
Thanks Diana for the review.
It seems the thumb instruction selection will take more work and needs probably split up into smaller ones (as discussed with Diana). So they will come later.
Nov 3 2017
Added thumb tests to all three. Also renamed the files to make it clear the test run both arm and thumb
Yes I too was thinking of adding just the run line, but I don't know when we move to instruction-selection would/could we do the same.
And if not, the separation upfront might be a good idea. Please advise. I am ok either way.
Nov 2 2017
Would it make sense to add a test, so that any future changes doesn't undo this behaviour?
Full context is missing (seems you forgot diff .... -U9999)
Oct 30 2017
Thanks for the work. However, in its current form the implementation is bit hard to follow. Would it be possible to, describe -
- Your overall approach in the implementation
2.. Describe above the function what it is trying to do (e.g. checkedGetIncrement - which b.t.w looks non-intuitive name).
Oct 29 2017
Thanks Diana for the feedback. I added the missing tests and they now all pass.