LGTM

LGTM, thanks.

In D106722#2905390, @fhahn wrote:

LGTM, thanks!

thanks @MaskRay

In the committed version, I added a new assert that should have caught this.

Address Florian's comments.

LGTM!

LGTM!

Congrats ;). Yes, I will commit it for you. Can you please rebase and retest, looks like the patch is not applying cleanly.

Address Florian's comments. Also fix the testcase that I copied mine from re: --check-prefix=RM

Looks great, thanks!

Glad, you were able to get back to this.

How about adding a total count as well? That should help quickly eye ball the ratios.

LGTM, you may want to wait a little for other potential comments. Especially some ideas on getting the actual mnemonic.

LGTM too.

In D79097#2218765, @hnrklssn wrote:

In D79097#2218587, @anemet wrote:

In D79097#2218567, @hnrklssn wrote:

In D79097#2218463, @anemet wrote:

@hnrklssn hi, are you panning to get back to this at some point?

Yes, sorry about the delay!
Currently finishing the last bits of the thesis that this work is part of has taken priority, which doesn't leave too much time since I'm now also working full time.
After that's done though I intend to come back and do the cleanup necessary to upstream as much of the work as possible. It's all related to improving optimisation remarks in one way or another. :)

Oh cool, are you planning to publish/publicize this work once it's done?

Yup, it will be published in LUP Student Papers afaict! I can send you a rough draft if you're interested. Most of the final content is in place already, but it's a bit rough around the edges.

In D79097#2218567, @hnrklssn wrote:

In D79097#2218463, @anemet wrote:

@hnrklssn hi, are you panning to get back to this at some point?

Yes, sorry about the delay!
Currently finishing the last bits of the thesis that this work is part of has taken priority, which doesn't leave too much time since I'm now also working full time.
After that's done though I intend to come back and do the cleanup necessary to upstream as much of the work as possible. It's all related to improving optimisation remarks in one way or another. :)

@hnrklssn hi, are you panning to get back to this at some point?

Looks reasonable to me.

LGTM.

In D77550#2151655, @anemet wrote:

BTW, will this be shared between .cpp's? I.e. what is the justification for a header?

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.

In D76867#2156196, @fhahn wrote:

In D76867#2154836, @anemet wrote:

Is this used anywhere in the patch? If not please explain the plan in the description.

I've updated the O0 case to use it (just committed that today). D76858 will use it in the backend pipeline. I think the description should already mention that the intention is to use it in the backend pipeline, without having to compute DT/AA there. Should I add more details?

Is this used anywhere in the patch? If not please explain the plan in the description.

BTW, will this be shared between .cpp's? I.e. what is the justification for a header?

LGTM.

LGTM! @nicolasvasilache can you please OK the MLIR parts if you're happy with them?

LGTM

Is there a test for passing alignment?

Sorry about the delay, just a few small nits on the tests, this is pretty close. Thanks for pushing this!

Thanks for improving this. Can you please add a small test highlighting where your change improve the diagnostics? TIA!

LGTM!

LGTM!

Nice, LGTM!

TODO: additional tests.

Very nice!

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...

LGTM

Can you please describe the approach in the description/in a comment?

LGTM, thanks for splitting these up like this!

LGTM

In the description when you say:

LGTM

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".

LGTM.

LGTM!

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.

LGTM, however it would better if this patch also converted existing code to use this new API.

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.

LGTM.

LGTM.

LGTM

LGTM

LGTM.

I am confused about this patch and the previous one (D70899). Looks like they both introduce VisitLoad. Is this intentional?

LGTM!

In D70897#1769062, @fhahn wrote:

In D70897#1768320, @anemet wrote:

Also the existing test diffs are hard to read, please explain what's going on there.

I've added comments to break up the check lines

We should be able to use fmuladd instead of fmul/fadd, as this just
reduces the rounding error between operations.