Refactor Linear Algebra's matmul to use a safer scratch buffer

[danielwe]

https://github.com/JuliaLang/julia/blob/54adf2193a6ea1b10cd1a184a7ba863aa985eede/stdlib/LinearAlgebra/src/matmul.jl#L536
https://github.com/JuliaLang/julia/blob/54adf2193a6ea1b10cd1a184a7ba863aa985eede/stdlib/LinearAlgebra/src/matmul.jl#L558

I stumbled upon these two comments while perusing the base linalg code. Related statement in JuliaLang/julia#23914, where they were introduced:

Also marked a few even more invalid places where managed pointer is passed to unsafe_wrap (the fix for those will be much more involved).

Not terribly reassuring seeing this in production code, so I figured it deserved an open issue.

[mbauman]

Discussed a few days ago on the #gripes slack channel. Marked in JuliaLang/julia#23914, invalid because "it’s pointing at an unaligned array though, and probably expecting to use simd instructions on it"

[mbauman]

Could you please explain the close, @yuyichao? I think this issue is useful to track the need to change this code.

[StefanKarpinski]

Reopening until we have an explanation or a fix.

[yuyichao]

The Base code is full of these. If you want to make a list of all the files to scan for any invalid use it's fine. Otherwise, this is just an issue with a uninformative title that can never be closed.

Also alignment isn't the issue the issue is explained in the original issue as

managed pointer is passed to unsafe_wrap

quoted above.

[mbauman]

In this specific case, though, the managed pointer is bound to a const global — it will always exist (edit: ah, unless it gets reshaped, I suppose). So (barring that) is this specific case really an issue if there's no alignment problem?

[StefanKarpinski]

Just because we don't currently have a complete set of issues for broken things with comments does not mean that we should not try to have issues for them. Comments tend not to get fixed or found as effectively as issues in an issue tracker. This is why we have issue trackers instead of just leaving comments all over the code. The issue tracker also has the benefit of supporting discussions like this one which help resolve and/or clarify the problem. In short, lack of a complete set of issues is not a justification for having no issue in this case.

[yuyichao]

Conceptually this is never allowed. Practically there are many compiler limitations and that's exactly why we have this issue in the first place.
Practically, I don't think this will be too bad with current compiler though future assumptions on aliasing could make this very bad. There is another issue with this code though that's unrelated with GC, it's not thread safe/reentrant.

lack of a complete set of issues is not a justification for having no issue in this case.

No. There has been many vague issues like this that are just left open until it's hard to tell if the issue is fixed or not. That's also why a uninformative title doesn't help since people will be opening duplicates and that go directly against keeping track of discussion. I don't want to rename it since I don't know what's the expected scope. It's a very broad issue but the issue for this specific case is also somewhat special as mentioned above. Now it's renamed and if people are actually interested in fixing it now, I have no problem keeping it open.

[danielwe]

Thanks for the rename, I lack the chops to fully understand the problem and write an appropriate title myself.

Based on a quick search there aren't that many fixme comments, and these two stand out as both particularly dramatic and opaque, and right in the middle of a core number crunching routine, so an issue seemed appropriate -- if only to rewrite them as less disconcerting and more informative. No intentions of spamming your issue tracker. https://github.com/JuliaLang/julia/search?p=2&q=fixme&type=&utf8=%E2%9C%93

[yuyichao]

there aren't that many fixme comments

Yes, I gave up after adding a few...