|
|
Created:
11 years, 6 months ago by Alan Leung Chromium Modified:
11 years, 5 months ago CC:
angleproject-review_googlegroups.com Base URL:
http://angleproject.googlecode.com/svn/trunk/ Visibility:
Public. |
DescriptionLess memory intensive initializing of builtins.
R=alokp@chromium.org
Committed: https://code.google.com/p/angleproject/source/detail?r=2425
Patch Set 1 #Patch Set 2 : #
Total comments: 12
Patch Set 3 : #
Total comments: 1
Patch Set 4 : #
Total comments: 1
Patch Set 5 : #Patch Set 6 : #
Total comments: 6
Patch Set 7 : #
Total comments: 6
Patch Set 8 : #Patch Set 9 : #Patch Set 10 : #
Total comments: 8
Patch Set 11 : #Patch Set 12 : #Patch Set 13 : #Patch Set 14 : #
MessagesTotal messages: 36
Hey Alok I played around with the idea of not using the parser at all for builtin initialization. I tried to maintain the current structure in order to keep them easy to update. Again, I am not too comfortable with C++ so I am welcome to other suggestions besides macros. I have redo the measurements in Android again. It saves almost exactly the same amount of memory as the early table copying hack I sent out. It is probably not surprising that the runtime difference is insignificant. What's your take on this one?
Sign in to reply to this message.
A couple of questions. I am not sure where the memory savings are coming from. See my comments below. https://codereview.appspot.com/9436044/diff/1004/src/compiler/Initialize.cpp File src/compiler/Initialize.cpp (left): https://codereview.appspot.com/9436044/diff/1004/src/compiler/Initialize.cpp#... src/compiler/Initialize.cpp:24: TString s; The question is why did the memory usage reduce? I can think of two reasons: 1. We are not building the AST for common functions. I doubt this because these functions should not be placed into AST. 2. We are not building this huge TString, which allocates the memory for the internal char buffer using the GlobalPoolAllocator. I think it is most probably due to 2. If this is the case, this can be very easily fixed by using std::String instead of TSring. Can you use std::string for builtInStrings instead of TString and see if you get similar savings? https://codereview.appspot.com/9436044/diff/1004/src/compiler/Initialize.cpp File src/compiler/Initialize.cpp (right): https://codereview.appspot.com/9436044/diff/1004/src/compiler/Initialize.cpp#... src/compiler/Initialize.cpp:836: //builtInStrings.push_back(BuiltInFunctionsCommon(resources)); Why only build it for common functions? What is preventing us from doing it for others? https://codereview.appspot.com/9436044/diff/1004/src/compiler/Initialize.h File src/compiler/Initialize.h (right): https://codereview.appspot.com/9436044/diff/1004/src/compiler/Initialize.h#ne... src/compiler/Initialize.h:38: TType * t = 0; If you directly used the TBasicType enum, there would not be any need for string matching.
Sign in to reply to this message.
A couple of questions. I am not sure where the memory savings are coming from. See my comments below.
Sign in to reply to this message.
Please note that this code is undergoing significant changes for ES3 support, since it defines new shader builtins. These patches will be merged into the es3proto branch in a couple of weeks. That said I definitely like the idea of inserting the builtin signatures into the symbol table directly instead of by parsing them! Could you replace the macros with inline functions? Thanks.
Sign in to reply to this message.
Thanks for looking at this, Alok / Nicolas. As for nicolas's question: I thought about functions before. What is the advantage over macros? I went with macros because I can get away with not quoting the every parameter as a string. It sort of resembles the original code if you squint hard enough. https://codereview.appspot.com/9436044/diff/1004/src/compiler/Initialize.cpp File src/compiler/Initialize.cpp (left): https://codereview.appspot.com/9436044/diff/1004/src/compiler/Initialize.cpp#... src/compiler/Initialize.cpp:24: TString s; On 2013/05/16 04:19:37, Alok Priyadarshi wrote: > The question is why did the memory usage reduce? I can think of two reasons: > 1. We are not building the AST for common functions. I doubt this because these > functions should not be placed into AST. > 2. We are not building this huge TString, which allocates the memory for the > internal char buffer using the GlobalPoolAllocator. > > I think it is most probably due to 2. If this is the case, this can be very > easily fixed by using std::String instead of TSring. Can you use std::string for > builtInStrings instead of TString and see if you get similar savings? I think it is a bit of both according to the profiling I have done. I have seen lots of pages getting allocated for both the strings as well as artifacts needed by the parser. Looking at the profile sometimes isn't very telling thought, because I only see where new pages needed to be allocated but not the real distribution of objects. https://codereview.appspot.com/9436044/diff/1004/src/compiler/Initialize.h File src/compiler/Initialize.h (right): https://codereview.appspot.com/9436044/diff/1004/src/compiler/Initialize.h#ne... src/compiler/Initialize.h:38: TType * t = 0; On 2013/05/16 04:19:37, Alok Priyadarshi wrote: > If you directly used the TBasicType enum, there would not be any need for string > matching. I still need to know the dimension of the vector as well as setting matrix=true/false right?
Sign in to reply to this message.
https://codereview.appspot.com/9436044/diff/1004/src/compiler/Initialize.cpp File src/compiler/Initialize.cpp (left): https://codereview.appspot.com/9436044/diff/1004/src/compiler/Initialize.cpp#... src/compiler/Initialize.cpp:24: TString s; Can you try just using std::string instead of TString for this and check how much saving you get? https://codereview.appspot.com/9436044/diff/1004/src/compiler/Initialize.cpp File src/compiler/Initialize.cpp (right): https://codereview.appspot.com/9436044/diff/1004/src/compiler/Initialize.cpp#... src/compiler/Initialize.cpp:836: //builtInStrings.push_back(BuiltInFunctionsCommon(resources)); On 2013/05/16 04:19:37, Alok Priyadarshi wrote: > Why only build it for common functions? What is preventing us from doing it for > others? Any reason you are only doing it for common functions? https://codereview.appspot.com/9436044/diff/1004/src/compiler/Initialize.h File src/compiler/Initialize.h (right): https://codereview.appspot.com/9436044/diff/1004/src/compiler/Initialize.h#ne... src/compiler/Initialize.h:38: TType * t = 0; quite right. I am sure we can come up with a maintainable and efficient mechanism to do this mapping. I would first like to understand how we are able to save that much memory.
Sign in to reply to this message.
https://codereview.appspot.com/9436044/diff/1004/src/compiler/Initialize.cpp File src/compiler/Initialize.cpp (left): https://codereview.appspot.com/9436044/diff/1004/src/compiler/Initialize.cpp#... src/compiler/Initialize.cpp:24: TString s; On 2013/05/16 22:51:11, Alok Priyadarshi wrote: > Can you try just using std::string instead of TString for this and check how > much saving you get? Turned out to be very little. My guess is that the declaration code is under 8000 chars and not very big compared to the AST. https://codereview.appspot.com/9436044/diff/1004/src/compiler/Initialize.cpp File src/compiler/Initialize.cpp (right): https://codereview.appspot.com/9436044/diff/1004/src/compiler/Initialize.cpp#... src/compiler/Initialize.cpp:836: //builtInStrings.push_back(BuiltInFunctionsCommon(resources)); On 2013/05/16 22:51:11, Alok Priyadarshi wrote: > On 2013/05/16 04:19:37, Alok Priyadarshi wrote: > > Why only build it for common functions? What is preventing us from doing it > for > > others? > > Any reason you are only doing it for common functions? There is no real reason, just laziness :) I just wanted to get a quick proof of concept to work and see what kind of numbers we get from it. Just wanted to avoid wasted work if people don't feel this is a good change :) https://codereview.appspot.com/9436044/diff/1004/src/compiler/Initialize.h File src/compiler/Initialize.h (right): https://codereview.appspot.com/9436044/diff/1004/src/compiler/Initialize.h#ne... src/compiler/Initialize.h:38: TType * t = 0; On 2013/05/16 22:51:11, Alok Priyadarshi wrote: > quite right. I am sure we can come up with a maintainable and efficient > mechanism to do this mapping. I would first like to understand how we are able > to save that much memory. See earlier comment in this patch. The AST is currently pretty heavy on memory. Similar to my earlier SymbolTable cloning suggestion, we only need the symboltable that it generates and the AST make up around ~0.3MB. That's something we never need again.
Sign in to reply to this message.
I like the idea of manually building the built-in symbol table. Lets figure out how to do it in a easily extensible and efficient manner. I propose a combination of function dictionary and python script. Nicolas: Do you have any other suggestions? https://codereview.appspot.com/9436044/diff/1004/src/compiler/Initialize.h File src/compiler/Initialize.h (right): https://codereview.appspot.com/9436044/diff/1004/src/compiler/Initialize.h#ne... src/compiler/Initialize.h:89: #define BUILTIN1(t, rvalue, name, ptype1, pname1) \ I propose auto-generating a cpp file with these function calls. We could have a human-readable dictionary of all built-in functions and a python script that spits out these calls. We can discuss this in person if you want.
Sign in to reply to this message.
> combination of function dictionary and python script. > > Nicolas: Do you have any other suggestions? I basically wrote a Perl script to generate those function calls. I am happy to port it to Python. Should I bring this up in the mailing list and see what other think?
Sign in to reply to this message.
Yes please port it to python if you can.
Sign in to reply to this message.
On 2013/05/23 22:38:12, Alok Priyadarshi wrote: > Yes please port it to python if you can. What do you mean by a function dictionary? I am thinking of a text file just with a similar structure as this: // // Angle and Trigonometric Functions. // float radians(float, degrees) vec2 radians(vec2, degrees) vec3 radians(vec3, degrees) vec4 radians(vec4, degrees) float degrees(float, radians) vec2 degrees(vec2, radians) vec3 degrees(vec3, radians) vec4 degrees(vec4, radians) float sin(float, angle) vec2 sin(vec2, angle) vec3 sin(vec3, angle) vec4 sin(vec4, angle) ..... THoughts?
Sign in to reply to this message.
If you use a text file, you would need to parse it. I was thinking of a hard-coded python dictionary: [ { name: "radians", return_type: "float", parameters: [ type: "float", name: "degrees" ] }, ... ] Manually adding/removing entries from such a dictionary should be pretty easy and writing/maintaining the code to spit-out code to insert these entries into the symbol table would be straight-forward.
Sign in to reply to this message.
What do you think of something like this. It allows us to have finer control such as putting "if statements" in the definition file.
Sign in to reply to this message.
https://codereview.appspot.com/9436044/diff/18002/src/compiler/Initialize.h File src/compiler/Initialize.h (right): https://codereview.appspot.com/9436044/diff/18002/src/compiler/Initialize.h#n... src/compiler/Initialize.h:37: static TType* GetGlobalType(const std::string& name) { One of the goals of auto-generating the code to populate symbol-table is to not have these macros or string-matching during runtime. This should be resolved inside the script.
Sign in to reply to this message.
On 2013/05/28 21:50:01, Alok Priyadarshi wrote: > https://codereview.appspot.com/9436044/diff/18002/src/compiler/Initialize.h > File src/compiler/Initialize.h (right): > > https://codereview.appspot.com/9436044/diff/18002/src/compiler/Initialize.h#n... > src/compiler/Initialize.h:37: static TType* GetGlobalType(const std::string& > name) { > One of the goals of auto-generating the code to populate symbol-table is to not > have these macros or string-matching during runtime. This should be resolved > inside the script. Makes total sense. Let me work on that. But what do you think about the common.input file format? Does this sound like a good approach to you?
Sign in to reply to this message.
> But what do you think about the common.input file format? Does this sound like a > good approach to you? I would prefer that the file format easily supports all types that can be inserted into symbol-table, i.e., type-names, variables, and functions. If the proposed file-format supports it, I am fine with that. It seems like to support all three, the parser would become a bit involved.
Sign in to reply to this message.
On 2013/05/28 22:29:09, Alok Priyadarshi wrote: > > But what do you think about the common.input file format? Does this sound like > a > > good approach to you? > > I would prefer that the file format easily supports all types that can be > inserted into symbol-table, i.e., type-names, variables, and functions. If the > proposed file-format supports it, I am fine with that. It seems like to support > all three, the parser would become a bit involved. How about now?
Sign in to reply to this message.
Getting closer! I think the following scheme would work well: 1. builtin_symbols.json will contain a dictionary of all built-in symbols - types, variables, and functions 2. generate_builtin_symbol_table.py will parse builtin_symbols.json and produce builtin_symbol_table.[h,cpp] 3. builtin_symbol_table.[h,cpp] will contain function definition for inserting built-in symbols: InsertBuiltInSymbols(ShShaderType type, ShShaderSpec spec, const ShBuiltInResources& resources, TSymbolTable* symbolTable); https://codereview.appspot.com/9436044/diff/24001/src/compiler/Initialize.h File src/compiler/Initialize.h (right): https://codereview.appspot.com/9436044/diff/24001/src/compiler/Initialize.h#n... src/compiler/Initialize.h:37: #define BUILTIN1(t, rvalue, name, ptype1, pname1) \ Is there a reason for these macros to be here? Can they moved into the script as well?
Sign in to reply to this message.
Thanks for the suggestions, Alok. Would you mind taking another look? On 2013/05/29 05:43:19, Alok Priyadarshi wrote: > Getting closer! > > I think the following scheme would work well: > > 1. builtin_symbols.json will contain a dictionary of all built-in symbols - > types, variables, and functions Done. > 2. generate_builtin_symbol_table.py will parse builtin_symbols.json and produce > builtin_symbol_table.[h,cpp] Done. > 3. builtin_symbol_table.[h,cpp] will contain function definition for inserting > built-in symbols: > InsertBuiltInSymbols(ShShaderType type, > ShShaderSpec spec, > const ShBuiltInResources& resources, > TSymbolTable* symbolTable); > > https://codereview.appspot.com/9436044/diff/24001/src/compiler/Initialize.h > File src/compiler/Initialize.h (right): > > https://codereview.appspot.com/9436044/diff/24001/src/compiler/Initialize.h#n... > src/compiler/Initialize.h:37: #define BUILTIN1(t, rvalue, name, ptype1, pname1) > \ > Is there a reason for these macros to be here? Can they moved into the script as > well? I choose to have the C preprocessing explan the macros instead of me expanding it in the function. Or are you just suggesting that I put the definition in a generated file? I am not seeing a clear advantage. Thanks! -Alan
Sign in to reply to this message.
did you forget to include the json file? https://codereview.appspot.com/9436044/diff/27001/src/compiler/builtin_symbol... File src/compiler/builtin_symbol_table.cpp (right): https://codereview.appspot.com/9436044/diff/27001/src/compiler/builtin_symbol... src/compiler/builtin_symbol_table.cpp:1: // Copyright (c) 2002-2013 The ANGLE Project Authors. All rights reserved. Add a warning that this file is auto-generated. DO NOT EDIT. https://codereview.appspot.com/9436044/diff/27001/src/compiler/builtin_symbol... File src/compiler/builtin_symbol_table.h (right): https://codereview.appspot.com/9436044/diff/27001/src/compiler/builtin_symbol... src/compiler/builtin_symbol_table.h:11: #define BUILTIN1(t, rvalue, name, ptype1, pname1) \ These macros should be moved to builtin_symbol_table.cpp. They are not used elsewhere. https://codereview.appspot.com/9436044/diff/27001/src/compiler/builtin_symbol... src/compiler/builtin_symbol_table.h:40: void InsertBuiltInFunctionsCommon(const ShBuiltInResources& resources, TSymbolTable * t); This function should be extern.
Sign in to reply to this message.
I added the json file as well. https://codereview.appspot.com/9436044/diff/27001/src/compiler/builtin_symbol... File src/compiler/builtin_symbol_table.cpp (right): https://codereview.appspot.com/9436044/diff/27001/src/compiler/builtin_symbol... src/compiler/builtin_symbol_table.cpp:1: // Copyright (c) 2002-2013 The ANGLE Project Authors. All rights reserved. On 2013/05/30 18:49:48, Alok Priyadarshi wrote: > Add a warning that this file is auto-generated. DO NOT EDIT. Done. https://codereview.appspot.com/9436044/diff/27001/src/compiler/builtin_symbol... File src/compiler/builtin_symbol_table.h (right): https://codereview.appspot.com/9436044/diff/27001/src/compiler/builtin_symbol... src/compiler/builtin_symbol_table.h:11: #define BUILTIN1(t, rvalue, name, ptype1, pname1) \ On 2013/05/30 18:49:48, Alok Priyadarshi wrote: > These macros should be moved to builtin_symbol_table.cpp. They are not used > elsewhere. Good Point. Done. https://codereview.appspot.com/9436044/diff/27001/src/compiler/builtin_symbol... src/compiler/builtin_symbol_table.h:40: void InsertBuiltInFunctionsCommon(const ShBuiltInResources& resources, TSymbolTable * t); On 2013/05/30 18:49:48, Alok Priyadarshi wrote: > This function should be extern. Done.
Sign in to reply to this message.
Almost there. Did you forget to include changes to avoid adding common-function string? https://codereview.appspot.com/9436044/diff/32001/src/compiler/builtin_symbol... File src/compiler/builtin_symbol_table.h (right): https://codereview.appspot.com/9436044/diff/32001/src/compiler/builtin_symbol... src/compiler/builtin_symbol_table.h:9: #include "compiler/SymbolTable.h" No need to include SymbolTable.h anymore. Just fwd-declare ShBuiltInResources and SymbolTable. https://codereview.appspot.com/9436044/diff/32001/src/compiler/builtin_symbol... src/compiler/builtin_symbol_table.h:11: nit: extra line. https://codereview.appspot.com/9436044/diff/32001/src/compiler/builtin_symbol... File src/compiler/builtin_symbols.json (right): https://codereview.appspot.com/9436044/diff/32001/src/compiler/builtin_symbol... src/compiler/builtin_symbols.json:24: "common" : [ I think it is fine with the scope of your changes. But if we need to move over all built-in symbols, the dictionary needs to support all three types - built-in types, variables, and functions. This can be added in subsequent patches.
Sign in to reply to this message.
PTAL? > Did you forget to include changes to avoid adding common-function string? What do you mean? I don't fully follow. Do you mean that CL I have to avoid creating too much TStrings for mangled names? That's orthogonal to this work, no?
Sign in to reply to this message.
PTAL? https://codereview.appspot.com/9436044/diff/32001/src/compiler/builtin_symbol... File src/compiler/builtin_symbol_table.h (right): https://codereview.appspot.com/9436044/diff/32001/src/compiler/builtin_symbol... src/compiler/builtin_symbol_table.h:9: #include "compiler/SymbolTable.h" On 2013/05/31 03:47:58, Alok Priyadarshi wrote: > No need to include SymbolTable.h anymore. Just fwd-declare ShBuiltInResources > and SymbolTable. Done. https://codereview.appspot.com/9436044/diff/32001/src/compiler/builtin_symbol... src/compiler/builtin_symbol_table.h:11: On 2013/05/31 03:47:58, Alok Priyadarshi wrote: > nit: extra line. Done. https://codereview.appspot.com/9436044/diff/32001/src/compiler/builtin_symbol... File src/compiler/builtin_symbols.json (right): https://codereview.appspot.com/9436044/diff/32001/src/compiler/builtin_symbol... src/compiler/builtin_symbols.json:24: "common" : [ On 2013/05/31 03:47:58, Alok Priyadarshi wrote: > I think it is fine with the scope of your changes. > > But if we need to move over all built-in symbols, the dictionary needs to > support all three types - built-in types, variables, and functions. This can be > added in subsequent patches. Totally agree. I have no plan to stop working on this after this CL. But I find smaller patches easier to work with.
Sign in to reply to this message.
On 2013/06/03 20:40:51, acleung1 wrote: > PTAL? > > > Did you forget to include changes to avoid adding common-function string? > > What do you mean? I don't fully follow. > > Do you mean that CL I have to avoid creating too much TStrings for mangled > names? That's orthogonal to this work, no? I do not understand how this works if you still call BuiltInFunctionsCommon() in Initialize.cpp. I would expect that function to go away. Otherwise InsertBuiltInFunctionsCommon() will not do anything. I do not see any changes to Initialize.cpp in this patch. You had call to BuiltInFunctionsCommon commented out in earlier patches.
Sign in to reply to this message.
Please note that using a macro will create a lot of code. Also, this approach is creating the same TType many times. You could instead have a new symbol table method that looks like this: bool TSymbolTable::insertBuiltIn(TType *rvalue, const char *name, TType *ptype1, const char *pname1, TType *ptype2 = 0, const char *pname2 = 0, TType *ptype3 = 0, const char *pname3 = 0) { TFunction *function = new TFunction(NewPoolTString(name), *rvalue); TParameter param1 = {NewPoolTString(pname1), ptype1}; function->addParameter(param1); if (pname2) { TParameter param2 = {NewPoolTString(pname2), ptype2}; function->addParameter(param2); } if (pname3) { TParameter param3 = {NewPoolTString(pname3), ptype3}; function->addParameter(param3); } return insert(*function); } And use it like this: TType *float1 = new TType(EbtFloat, EbpUndefined, EvqGlobal, 1); TType *float2 = new TType(EbtFloat, EbpUndefined, EvqGlobal, 2); ... symbolTable.insertBuiltIn(float1, "radians", float1, "degrees"); symbolTable.insertBuiltIn(float2, "radians", float2, "degrees"); ... This also eliminates the need for the json file and the python script. Just a suggestion.
Sign in to reply to this message.
On 2013/06/03 20:46:32, Alok Priyadarshi wrote: > On 2013/06/03 20:40:51, acleung1 wrote: > > PTAL? > > > > > Did you forget to include changes to avoid adding common-function string? > > > > What do you mean? I don't fully follow. > > > > Do you mean that CL I have to avoid creating too much TStrings for mangled > > names? That's orthogonal to this work, no? > > I do not understand how this works if you still call BuiltInFunctionsCommon() in > Initialize.cpp. I would expect that function to go away. Otherwise > InsertBuiltInFunctionsCommon() will not do anything. > > I do not see any changes to Initialize.cpp in this patch. You had call to > BuiltInFunctionsCommon commented out in earlier patches. Ah I am missing Initialize.cpp. I added it back. Please take another look? How do you normally work on Angle? I use git for Chromium and manually copy my changes to a Angle SVN client. I ended up forgetting lots of files over time.
Sign in to reply to this message.
On 2013/06/04 23:32:57, acleung1 wrote: > On 2013/06/03 20:46:32, Alok Priyadarshi wrote: > > On 2013/06/03 20:40:51, acleung1 wrote: > > > PTAL? > > > > > > > Did you forget to include changes to avoid adding common-function string? > > > > > > What do you mean? I don't fully follow. > > > > > > Do you mean that CL I have to avoid creating too much TStrings for mangled > > > names? That's orthogonal to this work, no? > > > > I do not understand how this works if you still call BuiltInFunctionsCommon() > in > > Initialize.cpp. I would expect that function to go away. Otherwise > > InsertBuiltInFunctionsCommon() will not do anything. > > > > I do not see any changes to Initialize.cpp in this patch. You had call to > > BuiltInFunctionsCommon commented out in earlier patches. > > Ah I am missing Initialize.cpp. I added it back. Please take another look? > > How do you normally work on Angle? I use git for Chromium and manually copy my > changes to a Angle SVN client. I ended up forgetting lots of files over time. I have the ANGLE client inside chromium checkout. You first tell gclient to not pull ANGLE. You .gclient should look like this: solutions = [ { "name" : "src", "url" : "https://chromium.googlesource.com/chromium/src.git", "deps_file" : ".DEPS.git", "managed" : True, "custom_deps" : { "src/third_party/angle": None, }, }, ] Notice the entry for angle inside custom_deps. Then "cd src/third_party; git svn clone https://angleproject.googlecode.com/svn/trunk angle". Start hacking under src/third_party/angle. Upload patches using "git cl upload".
Sign in to reply to this message.
On 2013/06/04 19:51:32, nicolas wrote: > Please note that using a macro will create a lot of code. Also, this approach is > creating the same TType many times. You could instead have a new symbol table > method that looks like this: > > bool TSymbolTable::insertBuiltIn(TType *rvalue, const char *name, TType *ptype1, > const char *pname1, TType *ptype2 = 0, > const char *pname2 = 0, TType *ptype3 = 0, > const char *pname3 = 0) > { > TFunction *function = new TFunction(NewPoolTString(name), *rvalue); > > TParameter param1 = {NewPoolTString(pname1), ptype1}; > function->addParameter(param1); > > if (pname2) > { > TParameter param2 = {NewPoolTString(pname2), ptype2}; > function->addParameter(param2); > } > > if (pname3) > { > TParameter param3 = {NewPoolTString(pname3), ptype3}; > function->addParameter(param3); > } > > return insert(*function); > } > > And use it like this: > > TType *float1 = new TType(EbtFloat, EbpUndefined, EvqGlobal, 1); > TType *float2 = new TType(EbtFloat, EbpUndefined, EvqGlobal, 2); > ... > > symbolTable.insertBuiltIn(float1, "radians", float1, "degrees"); > symbolTable.insertBuiltIn(float2, "radians", float2, "degrees"); > ... > > This also eliminates the need for the json file and the python script. > > Just a suggestion. Good point about avoiding macro. I still like the json file approach because it may be easier to maintain, especially when we add other types of built-in symbols, some of which may only be relevant for specific shader types, spec, language version, etc. I think we can incorporate your suggestion for sharing TType in the script. Did you have any other concern?
Sign in to reply to this message.
https://codereview.appspot.com/9436044/diff/47001/src/compiler/Initialize.cpp File src/compiler/Initialize.cpp (left): https://codereview.appspot.com/9436044/diff/47001/src/compiler/Initialize.cpp... src/compiler/Initialize.cpp:503: builtInStrings.push_back(BuiltInFunctionsCommon(resources)); Could you also delete BuiltInFunctionsCommon if it is not used anymore? https://codereview.appspot.com/9436044/diff/47001/src/compiler/builtin_symbol... File src/compiler/builtin_symbol_table.cpp (right): https://codereview.appspot.com/9436044/diff/47001/src/compiler/builtin_symbol... src/compiler/builtin_symbol_table.cpp:12: #define BUILTIN1(t, rvalue, name, ptype1, pname1) { TFunction* f = new TFunction(new TString(name), *rvalue); TParameter param = {new TString(pname1), ptype1}; f->addParameter(param); t->insert(*f); } Nicolas has a good point about not using these macros. I think this could be a function as Nicolas described in his patch. We can work on sharing TType in future patches. https://codereview.appspot.com/9436044/diff/47001/src/compiler/builtin_symbol... File src/compiler/builtin_symbol_table.h (right): https://codereview.appspot.com/9436044/diff/47001/src/compiler/builtin_symbol... src/compiler/builtin_symbol_table.h:9: #include "GLSLANG/ShaderLang.h" Is there a reason for including this file here?
Sign in to reply to this message.
PTAL. https://codereview.appspot.com/9436044/diff/47001/src/compiler/Initialize.cpp File src/compiler/Initialize.cpp (left): https://codereview.appspot.com/9436044/diff/47001/src/compiler/Initialize.cpp... src/compiler/Initialize.cpp:503: builtInStrings.push_back(BuiltInFunctionsCommon(resources)); On 2013/06/05 21:26:05, Alok Priyadarshi wrote: > Could you also delete BuiltInFunctionsCommon if it is not used anymore? Done. https://codereview.appspot.com/9436044/diff/47001/src/compiler/builtin_symbol... File src/compiler/builtin_symbol_table.cpp (right): https://codereview.appspot.com/9436044/diff/47001/src/compiler/builtin_symbol... src/compiler/builtin_symbol_table.cpp:12: #define BUILTIN1(t, rvalue, name, ptype1, pname1) { TFunction* f = new TFunction(new TString(name), *rvalue); TParameter param = {new TString(pname1), ptype1}; f->addParameter(param); t->insert(*f); } On 2013/06/05 21:26:05, Alok Priyadarshi wrote: > Nicolas has a good point about not using these macros. I think this could be a > function as Nicolas described in his patch. We can work on sharing TType in > future patches. Yep. Once I got this patch submitted I'll do a profile again to see how much we save for sharing TTypes. https://codereview.appspot.com/9436044/diff/47001/src/compiler/builtin_symbol... File src/compiler/builtin_symbol_table.h (right): https://codereview.appspot.com/9436044/diff/47001/src/compiler/builtin_symbol... src/compiler/builtin_symbol_table.h:9: #include "GLSLANG/ShaderLang.h" On 2013/06/05 21:26:05, Alok Priyadarshi wrote: > Is there a reason for including this file here? ShBuiltInResources is actually a typedef. I am not sure if we can forward declare a typedef.
Sign in to reply to this message.
https://codereview.appspot.com/9436044/diff/47001/src/compiler/builtin_symbol... File src/compiler/builtin_symbol_table.cpp (right): https://codereview.appspot.com/9436044/diff/47001/src/compiler/builtin_symbol... src/compiler/builtin_symbol_table.cpp:12: #define BUILTIN1(t, rvalue, name, ptype1, pname1) { TFunction* f = new TFunction(new TString(name), *rvalue); TParameter param = {new TString(pname1), ptype1}; f->addParameter(param); t->insert(*f); } Sure. We can work on sharing TType in a later patch. But we should still remove these macros in this patch. It should be pretty straightforward.
Sign in to reply to this message.
On 2013/06/05 23:55:42, Alok Priyadarshi wrote: > https://codereview.appspot.com/9436044/diff/47001/src/compiler/builtin_symbol... > File src/compiler/builtin_symbol_table.cpp (right): > > https://codereview.appspot.com/9436044/diff/47001/src/compiler/builtin_symbol... > src/compiler/builtin_symbol_table.cpp:12: #define BUILTIN1(t, rvalue, name, > ptype1, pname1) { TFunction* f = new TFunction(new TString(name), *rvalue); > TParameter param = {new TString(pname1), ptype1}; f->addParameter(param); > t->insert(*f); } > Sure. We can work on sharing TType in a later patch. But we should still remove > these macros in this patch. It should be pretty straightforward. PTAL.
Sign in to reply to this message.
Forgot to send the comments as well. https://codereview.appspot.com/9436044/diff/47001/src/compiler/builtin_symbol... File src/compiler/builtin_symbol_table.cpp (right): https://codereview.appspot.com/9436044/diff/47001/src/compiler/builtin_symbol... src/compiler/builtin_symbol_table.cpp:12: #define BUILTIN1(t, rvalue, name, ptype1, pname1) { TFunction* f = new TFunction(new TString(name), *rvalue); TParameter param = {new TString(pname1), ptype1}; f->addParameter(param); t->insert(*f); } On 2013/06/05 23:55:42, Alok Priyadarshi wrote: > Sure. We can work on sharing TType in a later patch. But we should still remove > these macros in this patch. It should be pretty straightforward. Done.
Sign in to reply to this message.
On 2013/06/06 22:21:41, acleung1 wrote: > Forgot to send the comments as well. > > https://codereview.appspot.com/9436044/diff/47001/src/compiler/builtin_symbol... > File src/compiler/builtin_symbol_table.cpp (right): > > https://codereview.appspot.com/9436044/diff/47001/src/compiler/builtin_symbol... > src/compiler/builtin_symbol_table.cpp:12: #define BUILTIN1(t, rvalue, name, > ptype1, pname1) { TFunction* f = new TFunction(new TString(name), *rvalue); > TParameter param = {new TString(pname1), ptype1}; f->addParameter(param); > t->insert(*f); } > On 2013/06/05 23:55:42, Alok Priyadarshi wrote: > > Sure. We can work on sharing TType in a later patch. But we should still > remove > > these macros in this patch. It should be pretty straightforward. > > Done. LGTM. Before committing please make sure that WebGL conformance tests still pass. To verify: 1. build chrome with your ANGLE changes. 2. visit https://www.khronos.org/registry/webgl/sdk/tests/webgl-conformance-tests.html 3. run ogles/GL/build tests - all of them should pass
Sign in to reply to this message.
|