fix operators containing percent for VM usage #13536

krux02 · 2020-02-28T23:59:20Z

Short explanation, in the VM signed numbers are stored with an extended sign. Unsigned numbers do not have a sign and therefore no sign extension. Applying unsigned operations on numbers with with sign extensions will have different results than without those extensions. Also unsigned ops do not apply sign extension afterwards, even though signed numbers require the sign extension. The casting operations introduced in this PR will do the sign extension removal and reintroduction for the VM accordingly. For runtime behavior, casting is a noop and should not affect performance.

fixes #13513

krux02 · 2020-02-29T01:15:06Z

Somehow there is a breaking test. This is what breaks, but it also breaks on devel. But it would mean that the development branch is broken.

import times

echo parse("1", "YYYY", utc())

timotheecour · 2020-02-29T01:42:37Z

BTW I am too lazy to add a test right now.

just copy the one I wrote here: https://github.com/nim-lang/Nim/pull/13532/files#diff-750bc0bcbc3193d1a2854eed2d5ec5d1

krux02 · 2020-02-29T01:47:53Z

@timotheecour Thanks for the help, but that test isn't suitable. I am circumventing to fix semfold for +%, so I don't special case testing for it. Also I am not just fixing +% in isolation. There is a problem with all % operators. So I need to test all of them. But if you care, your test does pass on my machine.

lib/system/comparisons.nim

timotheecour · 2020-02-29T01:52:54Z

tests/concepts/t3330.nim

@@ -17,17 +17,17 @@ proc add[T](x: var seq[T]; y: openArray[T])
  first type mismatch at position: 1
  required type for x: var seq[T]
  but expression 'k' is of type: Alias
-proc add(result: var string; x: float)
+proc add(x: var string; y: cstring)


no action needed here but I see this kind of thing break a lot (where ordering is changed), may be good to start sorting sigmatch errors in compiler by some reliable criterion (simplest would be raw string sort) to avoid having to edit tests

yea it is annoying. But I don't know what the cause it. Is it that I changed some definitions that symbols get a different order when iterating them? Or is it because I introduce a bug into +% so that hash values are calculated differently? Or did I fix +% and hash values are calculated differently? I don't know. I am worried because I don't understand it.

not sure about the other points raised but I'm tracking this specific issue here #13538

lib/system/comparisons.nim

timotheecour · 2020-02-29T02:15:07Z

lib/system/comparisons.nim


-template `>=%`*(x, y: untyped): untyped = y <=% x
+proc `>=%`*(x, y: int): bool {.inline.} =


instead of introducing all those overloads (previous code had a single template for >=%) use this:

template `>=%`*[T](x, y: T): untyped = y <=% x

ditto with >%

timotheecour · 2020-02-29T02:46:09Z

The implementation that uses a proc instead of a magic might cause slower performance on non optimized builds.

from reading your diff, seems like performance impact should only be for VM, not RT, can you confirm?
As for VM performance for such ops, may be good to do a quick benchmark to see what's the damage.
If VM performance isn't negatively affected, getting rid of VM specific code sounds like a very nice cleanup

you can remove dead code that's introduced in this PR, eg if mLeU64 is no longer used, these:

compiler/ast.nim:617:6:    mLeU64, mLtU64,
compiler/ast.nim:687:6:    mLeU64, mLtU64,
compiler/ccgexprs.nim:608:7:  of mLeU64: applyFormat("((NU64)($1) <= (NU64)($2))")
compiler/forloops.nim:16:42:    mEqUntracedRef, mLeI, mLeF64, mLeU, mLeU64, mLeEnum,
compiler/guards.nim:20:34:  someLe = {mLeI, mLeF64, mLeU, mLeU64, mLeEnum,
compiler/jsgen.nim:579:7:  of mLeU64: applyFormat("($1 <= $2)", "($1 <= $2)")
compiler/semfold.nim:294:13:  of mLeU, mLeU64:
compiler/vmgen.nim:1081:21:  of mLePtr, mLeU, mLeU64: genBinaryABC(c, n, dest, opcLeu)

ditto with mLtU64 etc

and also you may want to add tests that exercise Compiler creates invalid out-of-range int8 constants. #13513

krux02 · 2020-03-01T12:57:20Z

This is the test that you can execute to verify that here is a problem to fix.

proc testUnsignedOps() =
  let a: int8 = -128
  let b: int8 = 127

  echo toHex(cast[uint16](b +% 1))
  echo toHex(cast[uint16](b -% -1))
  echo toHex(cast[uint16](b *% 2))
  echo toHex(cast[uint16](a /% 4))
  echo toHex(cast[uint16](a %% 7))

testUnsignedOps()
static:
  testUnsignedOps()

krux02 · 2020-03-01T15:08:41Z

Test failure is unrelated.

krux02 · 2020-03-01T15:21:52Z

compiler/jsgen.nim

+    mInt64ToStr: ["cstrToNimstr", "cstrToNimstr"],
+    mFloatToStr: ["cstrToNimstr", "cstrToNimstr"],
+    mCStrToStr: ["cstrToNimstr", "cstrToNimstr"],
+    mStrToStr: ["", ""]]


I had to remove the mMeU64 and mLtU64. Since I was already at it. I converted the comments of the array into proper named array members.

Araq · 2020-03-03T11:48:57Z

I'd be happier with templates instead of inline procs as inline procs are currently not inlined in debug builds. Also you seem to mix refactorings with bugfixes which makes everything harder and slower to review.

timotheecour · 2020-03-03T12:13:49Z

I'd be happier with templates instead of inline procs as inline procs are currently not inlined in debug builds

then maybe that should be fixed instead ? IMO {.inline.} procs should inline in debug builds (ie without -d:release), but we could have an option --nimIgnoreInline to ignore {.inline.}
no strong opinion either way

krux02 · 2020-03-03T12:18:37Z

I'd be happier with templates instead of inline procs as inline procs are currently not inlined in debug builds.

I can do it if you say that is how it should be done.

Also you seem to mix refactorings with bugfixes which makes everything harder and slower to review.

I only mix in those refactoring that are structurally integrated with the bugfixes. The other refactorings are part of my other already merged PR here: #13551

But please don't expect me to like this splitting of the contribution. I only do it to do you a favor. But for me it means that the amount of time that I have to deal the slow CI the review process is doubled. And then when when one of the PRs is merged I usually get merge conflics as the bug fixes and refactorings can't be separated perfectly. So I have to deal with the CI yet another time.

timotheecour · 2020-03-03T12:35:12Z

in this particular case I feel like those refactorings are directly related to the bugfix so it wouldn't make sense to split in 2 PRs
in other similar cases though, there's a better way than splitting in 2 PRs PR1,PR2 (and having to deal with a lot of overhead work on the side of the contributor especially when PR1 needs to address review comment which then affects PR2):
simply make sure there are refactoring-only commits and bugfix-only commits (and if neeed, add a note to reviewer to that effect); and if a change needs to be addressed, use the usual git rebase -i HEAD~4

it results in much less overhead on side of contributor than splitting in PR's.

Araq · 2020-03-03T13:29:06Z

MO {.inline.} procs should inline in debug builds (ie without -d:release), but we could have an option --nimIgnoreInline to ignore {.inline.}

Agreed, let's hope __always_inline (or whatever it is called) is portable enough...

timotheecour · 2020-03-03T23:53:49Z

Agreed, let's hope __always_inline (or whatever it is called) is portable enough...

=> let's discuss this in nim-lang/RFCs#198

I'd be happier with templates instead of inline procs as inline procs are currently not inlined in debug builds

that's not what I'm seeing, see nim-lang/RFCs#198: cgen generates
static N_INLINE(NI, fun2x__rxUfcN4ZvcApCx0waSVH8At10303b)(NI a) {. regardless of -d:danger or not,

however:

inline procs WILL be noticably slower than templates in debug builds (or anything with --stacktrace) unless [RFC] --stacktrace:noinline: up to 3X speedups with typically same stacktraces #13582 is merged and user passes the --stacktrace:noinline this enables
in debug builds (even with --stacktrace:off and regardless of [RFC] --stacktrace:noinline: up to 3X speedups with typically same stacktraces #13582) I've observed that templates will be a little bit faster than inline proc, but same speed with danger builds (and maybe -d:release, haven't checked); however IMO codegen can be improved to make inline proc and template same speed even for debug build by removing redundant assignment incurred by inline proc (with --stacktrace:noinline or --stacktrace:off)

krux02 changed the title ~~fix #13513~~ fix operators containing percent for VM usage Feb 29, 2020

krux02 mentioned this pull request Feb 29, 2020

[superseded] fix #13513 +% now gives correct values in semfold + VM #13532

Closed

timotheecour reviewed Feb 29, 2020

View reviewed changes

lib/system/comparisons.nim Outdated Show resolved Hide resolved

timotheecour reviewed Feb 29, 2020

View reviewed changes

lib/system/comparisons.nim Show resolved Hide resolved

timotheecour reviewed Feb 29, 2020

View reviewed changes

timotheecour mentioned this pull request Feb 29, 2020

[minor] sigmatch errors should be sorted, for reproducible errors #13538

Closed

krux02 requested a review from Araq March 1, 2020 13:21

krux02 commented Mar 1, 2020

View reviewed changes

krux02 added 10 commits March 2, 2020 11:39

fix nim-lang#13513

f0cc5dd

fix test

8068013

fix another test

6a22c86

fix comment

5458ace

remove unused compiler magics

f10c9eb

add test

1722755

small cleanup

11cfb78

stuff

a3ad1ad

minor test cleanup

64a93c7

merge tarithmetics in tarithm

1b5a28f

krux02 force-pushed the fix-unsigned-operators branch from e9f1913 to 1b5a28f Compare March 2, 2020 10:39

Merge branch 'devel' into fix-unsigned-operators

2cfad93

krux02 mentioned this pull request Mar 3, 2020

Allocation free printing: remove write(a: varargs[string]) nim-lang/RFCs#197

Closed

This was referenced Mar 3, 2020

[RFC] {.inline.} guarantees and opt-out option timotheecour/Nim#48

Closed

[RFC] [performance] {.inline.} guarantees, --stacktrace:inline, and --ignoreInline nim-lang/RFCs#198

Open

Araq merged commit 2f55765 into nim-lang:devel Mar 11, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix operators containing percent for VM usage #13536

fix operators containing percent for VM usage #13536

krux02 commented Feb 28, 2020 •

edited

Loading

krux02 commented Feb 29, 2020

timotheecour commented Feb 29, 2020

krux02 commented Feb 29, 2020

timotheecour Feb 29, 2020 •

edited

Loading

krux02 Feb 29, 2020

timotheecour Feb 29, 2020

timotheecour Feb 29, 2020

timotheecour commented Feb 29, 2020 •

edited

Loading

krux02 commented Mar 1, 2020

krux02 commented Mar 1, 2020

krux02 Mar 1, 2020

Araq commented Mar 3, 2020

timotheecour commented Mar 3, 2020 •

edited

Loading

krux02 commented Mar 3, 2020

timotheecour commented Mar 3, 2020

Araq commented Mar 3, 2020 •

edited

Loading

timotheecour commented Mar 3, 2020 •

edited

Loading


		template `>=%`*(x, y: untyped): untyped = y <=% x
		proc `>=%`*(x, y: int): bool {.inline.} =

fix operators containing percent for VM usage #13536

fix operators containing percent for VM usage #13536

Conversation

krux02 commented Feb 28, 2020 • edited Loading

krux02 commented Feb 29, 2020

timotheecour commented Feb 29, 2020

krux02 commented Feb 29, 2020

timotheecour Feb 29, 2020 • edited Loading

Choose a reason for hiding this comment

krux02 Feb 29, 2020

Choose a reason for hiding this comment

timotheecour Feb 29, 2020

Choose a reason for hiding this comment

timotheecour Feb 29, 2020

Choose a reason for hiding this comment

timotheecour commented Feb 29, 2020 • edited Loading

krux02 commented Mar 1, 2020

krux02 commented Mar 1, 2020

krux02 Mar 1, 2020

Choose a reason for hiding this comment

Araq commented Mar 3, 2020

timotheecour commented Mar 3, 2020 • edited Loading

krux02 commented Mar 3, 2020

timotheecour commented Mar 3, 2020

Araq commented Mar 3, 2020 • edited Loading

timotheecour commented Mar 3, 2020 • edited Loading

krux02 commented Feb 28, 2020 •

edited

Loading

timotheecour Feb 29, 2020 •

edited

Loading

timotheecour commented Feb 29, 2020 •

edited

Loading

timotheecour commented Mar 3, 2020 •

edited

Loading

Araq commented Mar 3, 2020 •

edited

Loading

timotheecour commented Mar 3, 2020 •

edited

Loading