- User Since
- Jul 21 2014, 12:07 PM (321 w, 6 d)
Aug 14 2020
@hnrklssn hi, are you panning to get back to this at some point?
Jul 29 2020
Looks reasonable to me.
Jul 20 2020
I guess they could in theory also be added directly to LowerMatrixIntrinsics.cpp. But I think it makes sense to have a separate file for various matrix-related utilities that are not directly tied to the intrinsics. I don't anticipate users outside of LowerMatrixIntrinsics.cpp anytime soon upstream, so I could also move them there. I think in the long-term we want to specify specialized, target-specific lowerings outside of LowerMatrixIntrinsics.cpp, so we might as well start with more generally accessible helpers.
Jul 17 2020
Jul 15 2020
Is this used anywhere in the patch? If not please explain the plan in the description.
Jul 14 2020
BTW, will this be shared between .cpp's? I.e. what is the justification for a header?
Jul 13 2020
Jul 8 2020
Jun 16 2020
LGTM! @nicolasvasilache can you please OK the MLIR parts if you're happy with them?
Jun 15 2020
Is there a test for passing alignment?
May 13 2020
Sorry about the delay, just a few small nits on the tests, this is pretty close. Thanks for pushing this!
Apr 29 2020
Thanks for improving this. Can you please add a small test highlighting where your change improve the diagnostics? TIA!
Apr 28 2020
Apr 5 2020
Mar 31 2020
Mar 29 2020
TODO: additional tests.
Mar 23 2020
I have a few specific comments below but overall it would be great if we could simplify VisitBBFusion to avoid recursion and invalidating the iterator...
Mar 19 2020
Can you please describe the approach in the description/in a comment?
LGTM, thanks for splitting these up like this!
Mar 11 2020
In the description when you say:
Feb 26 2020
Jan 31 2020
On the description: I would first explain what you do and then how to do, i.e. have "To motivate" and the example before the paragprah "For a given function, we traverse".
Jan 23 2020
I think that in the description/commit log you want to say a bit on the motivation, like why it's useful to observe these matrix expression in this form.
Jan 17 2020
LGTM, however it would better if this patch also converted existing code to use this new API.
Jan 8 2020
As Francis mentioned it before it would be good derive the pass name from the remark type (diag::remark_gvn_load_elim -> gvn) . I.e. I would drop the DEBUG_TYPE argument.
Jan 7 2020
Dec 20 2019
Dec 19 2019
I am confused about this patch and the previous one (D70899). Looks like they both introduce VisitLoad. Is this intentional?
Dec 17 2019
We should be able to use fmuladd instead of fmul/fadd, as this just
reduces the rounding error between operations.
Dec 12 2019
Dec 9 2019
Dec 5 2019
LGTM too. You may want to a wait a few days to give other people a chance to comment further.
Dec 3 2019
Also the existing test diffs are hard to read, please explain what's going on there.
Nov 20 2019
Oct 29 2019
Sep 24 2019
Sep 20 2019
Sep 17 2019
Some minor test questions/suggestions. Feel free to commit after addressing.
Sep 6 2019
Does this also apply to right shifts?
Aug 20 2019
No, concerns. Looks good to me too.
Apr 18 2019
Mar 27 2019
LGTM, thanks for fixing this!
Mar 26 2019
Mar 14 2019
Feb 26 2019
Feb 8 2019
Feb 6 2019
LGTM with the comment.
Jan 25 2019
This is great, please add some tests or check for remarks in existing tests (e.g. for the recursive case).
Awesome and thanks for the test. LGTM!
Jan 24 2019
Can you please describe the user experience?
Dec 13 2018
Actually this has been failing for 8 hours. So reverted in r349117. Also reverted your attempt to update the test. It wasn't updating the right test: r349118