Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Inefficient codegen for varargs, openArray #22248

Open
arnetheduck opened this issue Jul 10, 2023 · 4 comments
Open

Inefficient codegen for varargs, openArray #22248

arnetheduck opened this issue Jul 10, 2023 · 4 comments
Labels
refc refc issues

Comments

@arnetheduck
Copy link
Contributor

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
Copy link
Member

Araq commented Jul 10, 2023

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

@arnetheduck
Copy link
Contributor Author

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
Copy link
Member

Araq commented Jul 10, 2023

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
Copy link
Contributor Author

arnetheduck commented Jul 11, 2023

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.

@ringabout ringabout added the refc refc issues label Sep 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refc refc issues
Projects
None yet
Development

No branches or pull requests

3 participants