- User Since
- May 9 2013, 11:10 AM (413 w, 2 d)
Wed, Apr 7
Fri, Mar 26
For the non XFAIL tests, I suggest to use the usual python script to auto generate CHECK directives.
Thu, Mar 25
LGTM modulo a few nits (see comments below).
Wed, Mar 24
Tue, Mar 23
LGTM. Thanks Andrew!
Mon, Mar 22
Be careful about referencing stack locations below the red-zone.
Fri, Mar 19
I personally don't like the idea of making the RCU optional in the RetireStage. The is no point in having a RetireStage if there is no RCU.
Wed, Mar 17
Thanks a lot Andrew.
Tue, Mar 16
Mon, Mar 15
There is nothing wrong with using a dummy RCU for the InOrderIssue stage.
Mar 11 2021
Mar 10 2021
Mar 9 2021
Ops.. I have accidentally "accepted" this patch.
Basically it LGTM if you remove the SDIV tests for now as I don't think that there is value in having them (unless you prove me wrong).
One last question.
What is the plan with the SDIV test cases? I don't think that there is anything that we can do to improve that simulation, since it would require knowledge that isn't available at simulation time. The risk is to end up with a test which isn't very useful in practice (it will always be marked as XFAIL). In which case, I suggest to remove those DIV tests entirely.
Mar 8 2021
Mar 2 2021
Feb 22 2021
Not sure why this review wasn't automatically closed.
This was committed back in August as cf6adecd6a8718ee2737ca55e4cd938364b984cc
Feb 19 2021
Feb 17 2021
Now that https://reviews.llvm.org/D95954 has been fixed, there are only a few things left to do:
Feb 4 2021
No problem. Your change was good.
Thanks for fixing the warning.
Not sure I understand why you would need an approval now. You have already committed this change.
@plotfi could you please commit this?
This is a duplicate of D95321.
No idea why it is taking so long for that other patch to go in.
Jan 27 2021
Thanks for the updated patch.
Jan 25 2021
For the record, that warning was firstly introduced by another commit which was meant to be an NFC refactoring
in preparation for the most recent work that added support for the generation of JSON files.
Jan 21 2021
I can see that InstructionView has been moved outside of View.h into its own file, but I cannot see that new file anywhere in this diff.
Jan 20 2021
Jan 19 2021
Thanks for working on this feature!
Jan 18 2021
I have only skimmed through this patch once, however I think that you can fix the problem in https://reviews.llvm.org/D94605 without introducing your new field ResourceUses.
Jan 16 2021
I really like this JSON format. Thanks a lot!
Dec 18 2020
The json output is better now.
However there are things about the implementation that must be fixed/changed (see my comments below).
Nov 25 2020
Thanks for checking.
Nov 24 2020
I've only skimmed through the code, but it looks like a nice improvement.
A couple of minor nits. Otherwise the llvm-mca change looks good.
Nov 23 2020
Thanks for the updated patch.
Nov 18 2020
LGTM for llvm-mca.
Oct 31 2020
Oct 26 2020
Much better. Thanks a lot :)
The new predicate looks good. I only have a couple of nits (see below).
Oct 23 2020
So, it doesn't look like the lowering is doing anything odd with the operand sequence. It should be fine.
At this point, the only explanation is that the original predicate was never really executed for that write variant. So it was never tested. I don't see other alternatives honestly.
The predicates look good to me.
The new predicate looks good to me.
Could you please add more context to the diff? I don't remember how IsLdstsoScaledPredX2 was defined.
Oct 22 2020
The new predicate looks good.
Oct 21 2020
Oct 19 2020
Oct 17 2020
I have been thinking a bit more about this patch.
D89553 is surprisingly less complicated than I have originally thought it was. It definitely touches more files. However the majority of the changes are very mechanical (mostly boilerplate required for the new MCSchedPredicate to work). It is also a non-controversial change, since it doesn't modify the layout of MCInst.
Bonus point: by changing the signature of resolveVariantSchedClass() we potentially enable future developments in the area of MCSchedPredicate (and mca). Basically that change would make it possible for users to declare new predicates on MCinstrDesc (which is definitely not a bad thing!).
The patch is mostly a mechanical change (i.e. a lot of boilerplate to introduce a new scheduling predicate).
Oct 16 2020
For the record, this is how your patch would look like if you decided to add a new predicate.
I am just trying to be helpful by showing how the alternative approach would look like (in case, if people decide that it is not a good idea to add an extra field to MCInst).
Oct 15 2020
are you still looking at this patch? Do you have any updates?
Sep 2 2020
Looks good to me.
Aug 28 2020
Aug 26 2020
Thanks Wolfgang for working on this.
Aug 25 2020
Looks good to me module a few nits (see below).
Aug 21 2020
Aug 20 2020
Aug 19 2020
Thanks Wolfgang! I always wanted to add a structured output to llvm-mca.
Thanks for working on this.
Aug 12 2020
Cheers. Now I have a better understanding of why you wanted to check latencies.