nim-lang / Nim

Nim is a statically typed compiled systems programming language. It combines successful concepts from mature languages like Python, Ada and Modula. Its design focuses on efficiency, expressiveness, and elegance (in that order of priority).
https://nim-lang.org
Other
16.47k stars 1.47k forks source link

Inefficient codegen for `varargs`, `openArray` #22248

Open arnetheduck opened 1 year ago

arnetheduck commented 1 year ago

Description

proc ff(a, b: string) = discard
proc fff(v: varargs[string]) = discard
proc fff2(v: openArray[string]) = discard

proc gggg() =
  let
    a = f()
    b = f()

  ff(a, b)
  fff(a, b)
  fff2([a, b])
N_LIB_PRIVATE N_NIMCALL(void, gggg__testit_12)(void) {
    NimStringDesc* a;
    NimStringDesc* b;
    tyArray__Re75IspeoxXy2oCZHwcRrA T1_;
    tyArray__Re75IspeoxXy2oCZHwcRrA T2_;
    a = f__testit_3();
    b = f__testit_3();
    ff__testit_5(a, b);
    nimZeroMem((void*)T1_, sizeof(tyArray__Re75IspeoxXy2oCZHwcRrA));
    T1_[0] = copyString(a);
    T1_[1] = copyString(b);
    fff__testit_8(T1_, 2);
    nimZeroMem((void*)T2_, sizeof(tyArray__Re75IspeoxXy2oCZHwcRrA));
    T2_[0] = copyString(a);
    T2_[1] = copyString(b);
    fff2__testit_10(T2_, 2);
}

clearly, the copyString is not needed

Nim Version

1.6.12, devel (refc)

Current Output

No response

Expected Output

No response

Possible Solution

No response

Additional Information

No response

Araq commented 1 year ago

ARC/ORC does not have this problem, maybe you should use it.

arnetheduck commented 1 year ago

ARC/ORC does not have this problem, maybe you should use it.

The possibility to not perform a copy here is an outcome of the rules of the language, not orc/arc specifically - if this is fixed for orc/arc but not for refc it would be an indication that the logic could benefit from being moved to a more appropriate abstraction level in the compiler thus improving the compiler architecture, decreasing maintenance overhead and simplifying the codebase - all this in addition to providing benefit to users that use the stable / released version of Nim and that might not be ready for months/years to migrate due to being heavy users of Nim and having a lot of code to re-check.

Araq commented 1 year ago

The logic was moved into an AST->AST transformation but the old refc code cannot easily make usage of these things because it's old code. And contrary to popular beliefs adding new logic ("features") does not break code but refactorings and bugfixes ("just a patch").

arnetheduck commented 1 year ago

well, the compiler is one, its users are many - when it has a bug like this, there's usually economy in fixing it in the compiler rather than requiring that all nim code out there to change to an unreleased development version of a new feature that has yet to go through real world testing - here, the generator is clearly buggy, making this a valid issue to report against refc that perhaps can be fixed and perhaps not.

What it does show however is a gap in the logic in the compiler somewhere: nobody benefits from those and they might be indicative of deeper issues.