zig icon indicating copy to clipboard operation
zig copied to clipboard

stage2: Miscellaneous fixes to vector arithmetic and copy elision

Open topolarity opened this issue 3 years ago • 7 comments

  • [Sema] Changes array initialization to scan for stores the same way struct init does
  • [Sema/Value] Renames compare (and family) to compareAll (etc.), and fixes lots of places where the wrong predicate for vectors was used
  • [Sema] If vector arithmetic yields a compile-time-known constant, make sure it's a vector
  • ~~[Liveness] Add a peephole optimization for safety checks to Liveness, for copy/load elision~~
  • [LLVM] Fix up load elision to account for "byref pass-through" consumers like .array_elem_val, etc. which do not make copies

P.S. As @Vexu pointed out to me, we still need a way to ensure that if, e.g., foo.arr[x][y].z performs a copy, it only copies the target object rather than all of foo. This is (usually) handled correctly already when the result is a non-by-ref type, but it needs some tightening, esp. for by-ref types.

Resolves #13064, Resolves #13065, Resolves #13069 + others mentioned in the commits

topolarity avatar Oct 05 '22 12:10 topolarity

Could you rebase against master branch please?

andrewrk avatar Oct 12 '22 18:10 andrewrk

~~I think cef0f11 might be exposing a bug in how we've been handling by_ref types in .struct_field_val~~

~~If I can't fix the issue, I'll have to revert this and make field/element accesses for unions and arrays perform a copy as well (meaning performance gets worse, unfortunately).~~

~~I'm going to convert this to a draft while I investigate.~~

All resolved now

topolarity avatar Oct 13 '22 21:10 topolarity

That Liveness change looks like it might deserve its own PR, I'd like to take a close look at it.

andrewrk avatar Oct 19 '22 20:10 andrewrk

That Liveness change looks like it might deserve its own PR, I'd like to take a close look at it.

Aye aye, I'll spin one up

topolarity avatar Oct 20 '22 04:10 topolarity

Made this dependent on https://github.com/ziglang/zig/pull/13221, which resolves the prior CI failures. CI passing is unfortunately still blocked on https://github.com/ziglang/zig/issues/13232, which is present on master but seems to be passing by the luck of the (alignment) draw ♠️

topolarity avatar Oct 20 '22 05:10 topolarity

Going to revisit this now that #13221 has landed.

topolarity avatar Nov 01 '22 15:11 topolarity

Is there an ETA on this? I was surprised these fixes did not go into 0.10.

billzez avatar Nov 01 '22 16:11 billzez

I like that approach! Seems like a nice improvement.

My sis got hitched last weekend 🎉 so I haven't gotten to fix this one up, but I should have it ready for review in a day or two.

topolarity avatar Nov 09 '22 18:11 topolarity

Thanks @topolarity !

billzez avatar Nov 11 '22 02:11 billzez