Reorganize profiles into their own files #5

TheSpydog · 2019-04-06T11:05:27Z

Couldn't get to sleep last night, so I did what any normal person would do -- clean up MojoShader! I ripped out all the profiles and put them in their own files in the profiles/ subdirectory. IMO this has been long overdue, but after having done it, I can totally see why nobody attempted it before. :P

A few things:

This works just fine in all of my tests, but it'd be good to make doubly sure I didn't break anything in the transition.
It might be worth a pass over the new header files to make sure the formatting looks okay. Beauty wasn't a very high priority when working on this initial commit...
We've got to figure out what to do about that mojoshader_internal.h error that I commented out. It's being fussy with the new files (probably due to some preprocessor ordering rule I'm not familiar with), but hopefully it won't be a difficult fix.
Since we're already overhauling the project structure, what are your thoughts on killing off that ARB1 target? TBH I don't even know what it is -- the top google search for "arb1 shader" was MojoShader's website. But I do know that removing it would simplify some things and a fix a small weird bit in the current implementation.

flibitijibibo · 2019-04-06T14:09:02Z

I’ll most likely start with a side-by-side comparison along with a binary size comparison. I’m hoping it’ll be exactly the same without debug symbols...
Any spots that are all new should be noted, just so we know what was actually added compared to the original.
Probably just need to define that value before including stuff in the profile c files.
I thought about this a lot, but only because the main file was so huge. ARB1 refers to ARB_vertex_program and ARB_fragment program, along with some NV shader extensions. Most (if not all) of this predates GLSL! This is how you can tell MojoShader was written in 2008... back then you needed these for good hardware support, but that’s not really true anymore. But I know Ryan likes his old hardware support, so I dunno if I want to mess with it. At least now it’s not as annoying to maintain.

TheSpydog · 2019-04-06T23:01:51Z

From my testing, the binary size (even when stripped) is quite a bit bigger than the previous version. I'm wondering if it's due to moving a lot of static inline functions out of mojoshader.c and into the mojoshader_profile_common.h where they're getting copied and pasted around several files.
The profile header files are the only major addition, but they were copied directly from their corresponding source files (with small changes to macros and such). Beyond that, I don't think there was anything significant added...
Yup, that was it.
Makes sense!

flibitijibibo · 2019-04-07T00:34:27Z

Very well, I’ll try to review the refactor tomorrow.

We’ll have to figure out the best path for dealing with the static functions. May have to macro, may have to throw them in common.c. That may have to be a separate commit though, since this changes enough as it is...

flibitijibibo

This was my first review pass. Overall it seems fine aside from style things, but I'm not crazy enough to expect perfect style from a 17k-line patch.

The main thing we need to document very very carefully in our final commit message is where everything moved. I tried marking spots that were clear transfers but then things fell apart a little since stuff got moved into profile_common, or were actually only used in certain spots, and some functions stayed as static inline while others did not, etc.

What might be the best next step is starting the process of replacing all the static inline functions with whatever is more appropriate now that it's all split up. Some of it could be macro-fied (i.e. all the one-liners) but that would mean editing even more code, so let's do that another time. What we can do now is try to group the related functions together and try to give them the same attributes. For example, if we have this...

static inline void Blorp1()
{
    lots;
    of;
    if (crap) {
       oh;
       good;
       grief;
    } // if
} // Blorp1

static inline int Blorp2(int zoop)
{
    return zoop == 69;
} // Blorp2

static inline int Blorp3(const char *why)
{
    return strcmp(why, "because");
} // Blorp3

... even though Blorp2/3 are trivial to inline, we have to be the bad guy and put all the Blorps in common.c together, instead of mixing/matching. Thankfully the only place where this got really messy was the register functions, everything else seemed clear.

The final version of this one patch will probably make the binary size a lot bigger, but fixing that can be a separate commit.

flibitijibibo · 2019-04-07T17:12:05Z

profiles/mojoshader_profile.h

+} Profile;
+
+
+// Common utilities...


Marking this as a change of mojoshader.c:301...

flibitijibibo · 2019-04-07T17:12:25Z

profiles/mojoshader_profile.h

+    ctx->free(ptr, ctx->malloc_data);
+} // Free
+
+int set_output(Context *ctx, Buffer **section);


Marking this as a change of mojoshader.c:342...

flibitijibibo · 2019-04-07T17:12:49Z

profiles/mojoshader_profile.h

+    return ctx->isfail;
+} // isfail
+
+void failf(Context *ctx, const char *fmt, ...);


Marking this as a change of mojoshader.c:420...

flibitijibibo · 2019-04-07T17:14:17Z

profiles/mojoshader_profile.h

+void floatstr(Context *ctx, char *buf, size_t bufsize, float f,
+                     int leavedecimal);
+
+RegisterList *reglist_insert(Context *ctx, RegisterList *prev,


Seeing lots of changes here compared to mojoshader.c:516...

flibitijibibo · 2019-04-07T17:14:57Z

profiles/mojoshader_profile.h

+    dst->writemask3 = ((mask >> 3) & 1);
+} // set_dstarg_writemask
+
+int allocate_scratch_register(Context *ctx);


Marking as a change of mojoshader.c:743...

profiles/mojoshader_profile_d3d.c

mojoshader.c

TheSpydog · 2019-04-07T19:38:40Z

All right, all the formerly static inline functions from mojoshader_profile.h are now non-static, non-inline functions housed in mojoshader_profile_common.c. I've additionally tried to organize things a bit better to match the original declaration order of mojoshader.c, although there are still some notable changes and/or omissions:

Several functions are only ever used in mojoshader.c, so they're still defined as static functions in that file:

free_reglist()
reglist_exists()
register_was_written()
get_defined_register()
add_attribute_register()
add_sampler()
adjust_token_position()

A couple functions turned out to only be used by the ARB1 profile, so I moved them into mojoshader_profile_arb1.c:

allocate_scratch_register()
allocate_branch_label()

And finally, get_used_register() was literally never used anywhere, so I just removed it.

Also, I merged the PREDECLARE_PROFILE macro calls into the existing #if SUPPORT_PROFILE_* stuff, per your comment.

TheSpydog · 2019-04-08T16:17:17Z

All right, with the help of Bloaty McBloatface and #pragma GCC visibility, I've managed to reduce the size difference between this version and the old version to a mere 56 bytes (when stripped and compiled with -DCOMPILER_SUPPORT=OFF).

Other notable change: usagestrs[] has been moved from mojoshader_profile.h to mojoshader_profile_d3d.c since only the D3D profile made use of this array.

TheSpydog · 2019-04-17T13:40:42Z

What's left to do here before this can be merged? Are we waiting on the SPIR-V emitter so we can refactor that into the new format too?

flibitijibibo · 2019-04-17T14:30:42Z

I'll do one last review of this today and if everything looks good we can make a Mercurial patch to push to upstream. EDIT: Have to push this to tomorrow, sorry!

flibitijibibo · 2019-04-23T17:38:12Z

profiles/mojoshader_profile.h

+int isscalar(Context *ctx, const MOJOSHADER_shaderType shader_type,
+             const RegisterType rtype, const int rnum);
+
+static const char swizzle_channels[] = { 'x', 'y', 'z', 'w' };


May be a possible cause of increased size, but just barely...

Yeah, it's annoying. I've tried moving it around and making it non-static, but no matter what it doesn't seem to affect the stripped binary size. :/

That's hopefully the optimizer being smart. Oh well, we can just leave this...

flibitijibibo · 2019-04-23T17:42:27Z

Passed this through all of my compilers and they seem to be happy with everything. The very last thing we get to do also happens to be the most tedious: We'll need to fix parameter lists for functions that used to be static but aren't anymore. One example is isscalar, which looked like this...

https://github.com/FNA-XNA/MojoShader/pull/5/files#diff-d496920d66adab7260e5d2670f334605L766

... but now looks like this:

https://github.com/FNA-XNA/MojoShader/pull/5/files#diff-f6bd4f8f8bd83197cca2564b822858d9R357

We just have to line everything up again. Once that's done we should be good to go on the Hg version of this patch.

TheSpydog · 2019-04-23T18:33:22Z

All right, I think I got 'em all. There could be a few that snuck by me, but we'll fix them as we see them.

Here's the hg patch (uploaded as a .txt file since GitHub doesn't like .patch files):
separateprofiles.txt

flibitijibibo · 2019-04-23T18:44:57Z

Merged into upstream:

https://hg.icculus.org/icculus/mojoshader/rev/b8ece252a201
cd04fa3

Moved all profiles to their own files

b6818a5

TheSpydog added 5 commits April 6, 2019 16:05

Fix __MOJOSHADER_INTERNAL__ define

5e98899

Disallow including mojoshader_profile_*.h from your applications

b0c8887

Added license notice to profile files

16af16a

Move profile loading to its original position

7752803

Remove redundant include

4d7e814

TheSpydog added 2 commits April 7, 2019 02:18

Merge branch 'fna' of https://github.com/FNA-XNA/MojoShader into fna

11ac5d1

Removed profile header files; generate declarations with macro instead

01c1de9

flibitijibibo reviewed Apr 7, 2019

View reviewed changes

flibitijibibo mentioned this pull request Apr 7, 2019

Finish SPIR-V emitter for MojoShader flibitijibibo/flibitBounties#1

Closed

Moved all (formerly) static inline functions to profile_common.c

dbe29be

TheSpydog added 2 commits April 8, 2019 12:09

Shrink binary size with #pragma GCC visibility

586d126

Keep reg_to_ui32 as static inline

8063b22

flibitijibibo reviewed Apr 23, 2019

View reviewed changes

TheSpydog added 2 commits April 23, 2019 13:57

Merge branch 'fna' of https://github.com/FNA-XNA/MojoShader into fna

a972ca6

Removed spacing left over from previously static functions

269e066

flibitijibibo closed this Apr 23, 2019

krolli mentioned this pull request Aug 26, 2019

SPIR-V Testing Thread #15

Closed

TheSpydog pushed a commit to TheSpydog/MojoShader that referenced this pull request May 24, 2020

Fix wrong offset and size when long is not 64-bit (icculus#5)

3f4b553

TheSpydog pushed a commit to TheSpydog/MojoShader that referenced this pull request May 26, 2020

Fix wrong offset and size when long is not 64-bit (icculus#5)

79a7145

TheSpydog pushed a commit to TheSpydog/MojoShader that referenced this pull request May 26, 2020

Fix wrong offset and size when long is not 64-bit (icculus#5)

c760ec3

TheSpydog pushed a commit to TheSpydog/MojoShader that referenced this pull request Jun 24, 2020

Fix wrong offset and size when long is not 64-bit (icculus#5)

bcb6eb3

TheSpydog pushed a commit to TheSpydog/MojoShader that referenced this pull request Jun 26, 2020

Fix wrong offset and size when long is not 64-bit (icculus#5)

5b98fc1

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reorganize profiles into their own files #5

Reorganize profiles into their own files #5

TheSpydog commented Apr 6, 2019

flibitijibibo commented Apr 6, 2019 •

edited

Loading

TheSpydog commented Apr 6, 2019

flibitijibibo commented Apr 7, 2019

flibitijibibo left a comment

flibitijibibo Apr 7, 2019

flibitijibibo Apr 7, 2019

flibitijibibo Apr 7, 2019

flibitijibibo Apr 7, 2019

flibitijibibo Apr 7, 2019

TheSpydog commented Apr 7, 2019

TheSpydog commented Apr 8, 2019

TheSpydog commented Apr 17, 2019

flibitijibibo commented Apr 17, 2019 •

edited

Loading

flibitijibibo Apr 23, 2019

TheSpydog Apr 23, 2019

flibitijibibo Apr 23, 2019

flibitijibibo commented Apr 23, 2019 •

edited

Loading

TheSpydog commented Apr 23, 2019

flibitijibibo commented Apr 23, 2019

Reorganize profiles into their own files #5

Reorganize profiles into their own files #5

Conversation

TheSpydog commented Apr 6, 2019

flibitijibibo commented Apr 6, 2019 • edited Loading

TheSpydog commented Apr 6, 2019

flibitijibibo commented Apr 7, 2019

flibitijibibo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheSpydog commented Apr 7, 2019

TheSpydog commented Apr 8, 2019

TheSpydog commented Apr 17, 2019

flibitijibibo commented Apr 17, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flibitijibibo commented Apr 23, 2019 • edited Loading

TheSpydog commented Apr 23, 2019

flibitijibibo commented Apr 23, 2019

flibitijibibo commented Apr 6, 2019 •

edited

Loading

flibitijibibo commented Apr 17, 2019 •

edited

Loading

flibitijibibo commented Apr 23, 2019 •

edited

Loading