- User Since
- Mar 27 2015, 6:20 AM (172 w, 6 d)
Fri, Jul 13
Mon, Jul 2
Is this applicable only for ARMv6 case? From the patch it does not seem so and so may be good to add cases for ARMv7 as well.
Fri, Jun 29
Thu, Jun 28
Jun 14 2018
Jun 13 2018
LGTM but will wait for others to comment as well before accepting
Jun 11 2018
Probably good to mention that this patch relies on https://reviews.llvm.org/D48013
Its not clear to me why 'GenericTable' is more efficient/better than SearchableTable. Maybe you could give some background in the commit message about this. I implemented the ARMSystemRegister bit sometime ago and found SearchableTable quite useful and concise but perhaps GenericTable/Enum is even better.
Jun 7 2018
but did not find a way to do this (I think that it didn't work to express a LSU#I SchedWrite). I guess the current above will have to do?
This will work.
Jun 6 2018
Jun 5 2018
Jun 4 2018
May 31 2018
May 30 2018
Should the commit message be " Splat floating-point immediate value to SVE vector". Or may be i am wrong on this?
May 29 2018
May 24 2018
May 23 2018
I am not an expert in encryption but how likely it is for 'key' to be zero ?
Other than that, LGTM
May 20 2018
May 17 2018
May 15 2018
LGTM but then I wrote it originally so better to wait for comments from others.
May 14 2018
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.
May 13 2018
May 12 2018
May 11 2018
May 10 2018
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.
May 9 2018
May 3 2018
Does FDIV use both the resources for 8 cycles? Writing [8,8] would imply that.
May 2 2018
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).
May 1 2018
Hi Kristof. Thanks for this.
Apr 30 2018
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.