General comments about latest v2023.03

[pps83]

I did review latest updates that were made since 33cf0d69a8e0fd5814481407064139d873aa8e06

• As usual, whatever editor you use, you should configure it to trim trailing whitespace.
• Old version used SSE2_ON and AVX2_ON defines to make multiple compilations of the same file, new version uses built-in __AVX2__ and __SSE3__, and that's not ok! I don't want to explain why it's wrong because it's better to fix the root cause and avoid the issue altogether: you should refactor the code so that the same library wouldn't need to compile the same input files multiple times with different compilation flags. That's quite unusual approach. And that's wrong imo, it makes it hard to maintain and reason about for others. What's even worse, is that not only your library needs to compile multiple files with different flags, these files also include themselves. Like, wtf, are you trying to confuse yourself so others gets confused thinking if you were confused while writing the code?.. (I did try to split all that hot mess once: https://github.com/pps83/TurboPFor/tree/master-split-tpl like here: https://github.com/pps83/TurboPFor/commit/194f1498372147bb7c37895fc6eaeef8657b7fe1)
• In short, TurboPFor requires two levels of refactoring: 1) avoid files including themselves (like transpose.c includes itself many times in the file itself), 2) don't stuff code for different arch in the same file so that you won't need to compile the same file with different compiler settings. VS2022 requires the second step, or, as I added, vs/transpose_avx2.c could be used as a workaround (the current approach). By removing AVX2_ON and relying directly on __AVX2__ the vs/transpose_avx2.c file now looks quite it simply includes transpose.c. Any reasonable person would be surprised: why not use transpose.c directly, which brings us back to the confusion loop. At least with AVX2_ON/SSE2_ON it was clear what was the intent and vs/transpose_avx2.c file wasn't simply including another file.
• It's good that there is now include dir, supposedly, it's the public include (right?). include_ is sketchy, better move them back to src three!
• VS2022 project looks messy (I'll make PRs later), header files aren't in the project (all are missing). If you try to build it, the project will always build (instead, it should skip all, as nothing needs to build).
• Looks like some fixes are reversed. For example, you have this code:

        if(unlikely(b!=8))
          *op &= (1ull<<(b*8))-1;         op++; ip += b;

  doesn't it look TERRIBLE?! Is it a bug (whole next line after if (...) should be wrapped with { ... }? Cause it looks like all that is a single statement, while it is not!

• Old code has stuff like TEMPLATE2, now it's changed to T2, T3. With many levels of inderection and overuse of preprocessor, this doesn't help readability imho.
• Seems like in some other places other changes got erased: _in[P4D_MAX+32]={0}, miss[P4D_MAX]={0} places fixes got removed. These became _in[P4D_MAX+32] and miss[P4D_MAX] (erasing changes made by 1f80e822050503dfc113208fe9aeee08104496ed). WTF?! These were added for some reason... we may pray perhaps and somebody will write it on the wall, what the change fixed back then. How can one be certain that you simply didn't revert the change just like that... or there was another change in the code that made the ={0} part unnecessary? In any case, all that must have been documented in the versioning. git commit messages exist not to put dots in the history, but to explain what and why was changed.

Could it be, that you took some outdated version of TurboPFor, and worked from there and that's why some fixes are removed now?

[pps83]

Also, you added files, made a separate commit for each such file like this one(d5d53edb6801e55ddea6dba43b14e638c9927815), then many commits later you remove these files from other dirs. That's looks quite odd, pretty bad project management. You should add and remove the files in a single commit. This way git knows that you moved the file, version history will be preserved. With your approach you simply obliterated commit history.

[powturbo]

Thank you for your comments and suggestions. I'm still refactoring TurboPFor.

• I'm using notepad++ on windows and the ubuntu standard editor on linux. I'm not aware why trailling spaces are causing any compiler issue. I think I've now removed all of them.
• I don't see any reason why not rely on the predefined compiler macros. There are no issues with gcc and clang on linux & windows. For vc++ just define __AVX2__ or _AVX__ / __SSE4.2_\. There are now only two modes AVX2 and non AVX2. SSE4.2 (sandy bridge) as a minimum. There is only one iclib version library including both AVX2 and non AVX2 functions.
• I'm not planing to remove the mode "files that including themselves". These files have common code sections for avx2,sse,neon... TurboPFor is supporting 8,16,32,64 bits integers (plain, sse,avx2, arm_neon,...) and for each size the integrated delta,delat1,xor,zigzag,... This will explode the number of files needed. With nearly 1000 functions, it makes it difficult for me to manage these sheer number of files. I know it's unusual like using gotos, but this is just fine for me. I want to have things as compact and managable as possible without duplicating common code sections. Nevertheless I'm planing to refactor some files like "transpose.c".
• TEMPLATE2/TEMPLATE3 are renamed to T2/T3. T3 is rarely used. People are usually using \<T> instead of \<TEMPLATE> in c++ and nobody is complaining.
• There is now only one public include "ic.h" (ic.h has mocros and is optional). "ic.h" is a copy of the files from "include" in one file. I'm thinking about to create a lib directory for the source files.
• I'll look at the reverted changes. The code you're showing (vsimple.c) is correctly formated when you look the same column on other lines. I'm not immediately updating the github repository with my local repository when I make changes over a long periode of time.

[pps83]

• There is now only one public include "ic.h" (ic.h has mocros and is optional). "ic.h" is a merge of the files from "include". I'm thinking about to create a lib directory for the source files.

Ah, so, effectively include duplicates what's inside include, right? I think I've seen code that does include something from `include`. I think it's better to cut off the old files instead of keeping them to avoid confusion. You can always retrieve them back from git if you need them

The code you're showing (vsimple.c) is correctly formated when you look the same column on other lines.

What do you mean? Are you referring to this column:

in any case, just look at the posted piece of code I provided above, it's clearly misleading. Should the op++; ip += b; piece be part of the if(...) {...} block or not? I wasn't digging the code for these issues, simply try to use clang and you'll see all these warnings.

I did remove these in prior PRs, now many such issues and real bugs will be back cause these changes are reversed all over the place. It's not so trivial to check entire project diff hunting for all these reverted changes. Couldn't you simply rebase whatever you were doing on top of latest to ensure that you aren't working with outdated code to avoid reverting changes?

[powturbo]

What do you mean? Are you referring to this column: Yes. Now it is on a separate line.

...working with outdated code to avoid reverting changes? ok

[pps83]

• I don't see any reason why not rely on the predefined compiler macros. There are no issues with gcc and clang on linux & windows. For vc++ just define AVX2 or AVX / SSE4.2.

Well, if you had absolute 100% control/monopoly on compilation of TurboPFor then there would be no issues. Somebody could have toolchain that always compiles for avx2 (eg always has __AVX2__ enabled), then building these two files will produce identical obj files and link errors. With a separate define to control that, you are 100% sure that whatever compiler does, you'll end up with what you expect.

[powturbo]

The current library is including both AVX2 and SSE versions. Most functions are either for sse (...128v) or avx2 (...256v) target. Some functions (like in transpose.c) are automatically set to the current cpu target. For other functions like in v8.c, avx2 is not faster than sse. There is no need to compile the whole project with AVX2. The predefined macros for ex. __AVX2__ are only available/defined if you use the target options at the compile command (for ex. -march=haswell or -mavx2 ). I see no difference and not a necessity to use separate macros/defines.

[pps83]

just look at this mess: https://github.com/powturbo/TurboPFor-Integer-Compression/blob/d254843f609a44727d6ae35c34c08fd72696a2b3/ext/beplugcs_.c#L42

hard to do this type of stuff even intentionally. Does that code look normal for you? You may wonder what's wrong with it.... well, try to scroll horizontally and you'll see.

https://raw.githubusercontent.com/powturbo/TurboPFor-Integer-Compression/d254843f609a44727d6ae35c34c08fd72696a2b3/ext/beplugcs_.c

[powturbo]

The files are used in icbench wich is actually no more used. I'm thinking about dropping totally icbench as it's no more maintained. The format errors are probably due to a global search/replace with notepad++. icapp is the only benchmark maintained at the moment.

[pps83]

c'mon, that's NOT OK: https://github.com/powturbo/TurboPFor-Integer-Compression/commit/d254843f609a44727d6ae35c34c08fd72696a2b3

The point wasn't to report that this single change was reverted so it gets fixed. Why would you make a separate commit for this?

I'll try to make it clear why I bothered you: when I updated, I did try to do a big diff and quickly review. And I simply happened to notice a change on the line that I recall used to trigger warnings with clang and I did a PR to fix it. Similarly, I saw other places where changes were reverted. So, to me it looked like you used some old version, you reverted some fixes by dumpling changes on top of git master branch without proper rebasing. I asked if that was something you really intended to do, and it looks like that wasn't the case. So, the issue seems clear and to address it, you'd need to check what else you reverted.

If you had proper commit history (TurboPFor- has some of the worst commit history) it would be possible to track the changes. Unfortunately, all the commits have absolutely USELESS messages. Even in this commit, what's the point of commit message "TurboPFor: Variable simple". TurboPFor is the name of the library, why do you need it there? "Variable simple" - is basically what the file is changed, vsimple.c is part of the commit itself. The commit message should say what and why is changed. There is NO WAY anything remotely close would be acceptable in a project where there are more than 1 ppl working. No way.

[pps83]

The files are used in icbench wich is actually no more used. I'm thinking about dropping totally icbench as it's no more maintained. The format errors are probably due to a global search/replace with notepad++. icapp is the only benchmark maintained at the moment.

I saw that long time ago, it won't even compile anymore. In my local branch I do have a project for VS that builds it. I'll push changes locally and if you want you may add it. But, perhaps, as you say, it's better to drop it if it wasn't maintained for a while. Also, would be a good idea to trim all unused stuff from that ext dir

[pps83]

I see no difference and not a necessity to use separate macros/defines.

In VS proj it's common/normal for devs to modify build settings however they like. By default, x64 builds with /arch:AVX. If I change to /arch:AVX2 project will fail. It certainly shouldn't fail if somebody targets it for avx2!! (If you try the same stuff with older version before you switched to use built-in macro for arch versioning I expect it to build when you change target instruction set in VS)

Also, Release/Debug Win32 are currently misconfigured, both produce tons of warnings and link errors.

[powturbo]

Actually I'm using git only to upload my local repository. It's a one man project and I'm only accepting small bug fixing / format changes. I'm not accepting any extension or extensive changes to the repository. I'm working on turbopfor only in my spare time when it permits.

I've also vs2022 and is building turbopfor without any error in debug and release mode.

I'm testing with vs2022, gcc/clang on windows and gcc/clang on linux and aarch64.

I've no issue compiling the project with the provided makefile including SEE/AVX2/ARM_NEON compiler switches. icapp is working just fine with arbitrary files.

Most users are building and using the turbopfor library in their programs. They are not changing any compiler option. The libic is configured to work on both AVX2 and SSE cpu targets. In general you can't change the target options if you have simd intrinsics in your project.

[pps83]

I've also vs2022 and is building turbopfor without any error in debug and release mode.

It won't build in Win32.

Check my https://github.com/pps83/TurboPFor, I have other changes on top, vs2022 builds both icapp and icbench (I added this project). I see that in your latest code you did similar changes to what I've done back in 2022: Move bitutil.h internal code to bitutil_in.h, remove BITUTIL_IN

I also did same stuff for vint, looks like your last version doesn't have it: Move vint.h internal code to vint_in.h, remove VINT_IN

Check the log: https://github.com/pps83/TurboPFor/commits/master Some should be merged imo: https://github.com/pps83/TurboPFor/commit/13aa7c3ab999af677f0f8f3ef61a63e6c058a7f0 https://github.com/pps83/TurboPFor/commit/f0559041e8b9bb2e241c8de401df26ed770bb747 https://github.com/pps83/TurboPFor/commit/a37f0e6b199d35881c38a15d5fb450a8a16a3cae https://github.com/pps83/TurboPFor/commit/756d5b9d00fd893746362377ef525fece8170a98 https://github.com/pps83/TurboPFor/commit/1a0405aee5042963a0f0341538424ae75f6048a9 https://github.com/pps83/TurboPFor/commit/fe1a737309c903c0967e43ed8b4ff8415dd34129 https://github.com/pps83/TurboPFor/commit/efa7118ffa72c3e2e1d21d1a53c88061727fe7f0 https://github.com/pps83/TurboPFor/commit/3cec9b08d2f56d33e19eb341554dc621e1eb6ba4 https://github.com/pps83/TurboPFor/commit/d292ba481eaba0a3d171a5c0b794e50a8b8cc3e2 https://github.com/pps83/TurboPFor/commit/35bbd38e21187f863ae1d455859f1acc628c4bd0