Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
883253003:
Add the mips_dsp_rev build variable. (Closed)
DescriptionAdd the mips_dsp_rev, mips_float_abi, and mips_fpu_mode build variables.
mips_dsp_rev specifies the MIPS DSP ASE (Application-Specific
Extension) revision number. Possible values are 0 (unavailable), 1,
and 2. Default to 0.
mips_float_abi specifies whether or not MIPS floating-point coprocessor
instructions should be used. Possible values are "hard" and "soft".
Default to "hard".
mips_fpu_mode specifies the width of MIPS32 floating-point registers.
Possible values are "fp32", "fp64", and "fpxx". Default to "fp32".
The companion webrtc CL is
https://webrtc-codereview.appspot.com/39779004.
R=brettw@chromium.org,kjellander@@chromium.org,petarj@mips.com
BUG=446234
Committed: https://crrev.com/ff66006e62449071ab87ad01b025dac4a9029d06
Cr-Commit-Position: refs/heads/master@{#318524}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Add the mips_float_abi and mips_fpu_mode build variables. #
Total comments: 21
Patch Set 3 : Make the changes suggested by Henrik and Brett #Patch Set 4 : Rebase after the GN cpu_arch => current_cpu renaming #
Total comments: 8
Patch Set 5 : Remove the mips_arch_variant!="r6" check for mips_float_abi. #
Total comments: 7
Patch Set 6 : Use variable substitutions in cflags. Consolidate declare_args. #Patch Set 7 : #Patch Set 8 : Adjust if statement nesting #
Total comments: 2
Messages
Total messages: 27 (6 generated)
Please review. webrtc's build system already has a mips_dsp_rev build variable. So I just moved the declaration of that variable from webrtc to Chromium. I also wanted to add a build variable for MIPS FPU, but I haven't figured out what it should be because v8 and webrtc use different build variables for MIPS FPU. I'd appreciate your advice on this. https://codereview.chromium.org/883253003/diff/1/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/883253003/diff/1/build/common.gypi#newcode2302 build/common.gypi:2302: 'mips_fpu_mode%': 'fp32', Petar: is the mips_fpu_mode build variable being used? It doesn't cause any compiler flag to be set inside this file. However, it is used heavily in v8/build/toolchain.gypi. I wonder if that the code in v8/build/toolchain.gypi that sets 'cflags' should be moved to this file.
https://codereview.chromium.org/883253003/diff/1/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/883253003/diff/1/build/common.gypi#newcode2302 build/common.gypi:2302: 'mips_fpu_mode%': 'fp32', On 2015/02/03 22:49:46, wtc wrote: > > Petar: is the mips_fpu_mode build variable being used? It doesn't cause any > compiler flag to be set inside this file. > > However, it is used heavily in v8/build/toolchain.gypi. I wonder if that the > code in v8/build/toolchain.gypi that sets 'cflags' should be moved to this file. We should copy the code that sets cflags based mips_fpu_mode here. Copy but not move, since v8 standalone build system should not be affected with the change.
Petar, Henrik: please review patch set 2, and the companion webrtc CL. I finally spent some time analyzing the v8 and webrtc build systems (both GYP and GN). Here are the results. We need two build variables to control these compiler flags: * -mhard-float vs. -msoft-float * -mfp32, -mfp64, or -mfpxx I annotated some of my changes. If you have questions about any change, please ask. I'll be happy to explain the change and point you to the corresponding code in v8 or webrtc. Thanks! https://codereview.chromium.org/883253003/diff/1/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/883253003/diff/1/build/common.gypi#newcode2302 build/common.gypi:2302: 'mips_fpu_mode%': 'fp32', On 2015/02/05 16:54:44, petarj wrote: > On 2015/02/03 22:49:46, wtc wrote: > > > > Petar: is the mips_fpu_mode build variable being used? It doesn't cause any > > compiler flag to be set inside this file. > > > > However, it is used heavily in v8/build/toolchain.gypi. I wonder if that the > > code in v8/build/toolchain.gypi that sets 'cflags' should be moved to this > file. > > We should copy the code that sets cflags based mips_fpu_mode here. > Copy but not move, since v8 standalone build system should not be affected with > the change. Done. https://codereview.chromium.org/883253003/diff/20001/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/883253003/diff/20001/build/common.gypi#newcod... build/common.gypi:2294: 'mips_float_abi%': 'hard', The conditional expression and the default value match the code for 'mips_fpu' in third_party/webrtc/build/common.gypi. https://codereview.chromium.org/883253003/diff/20001/build/common.gypi#newcod... build/common.gypi:4064: ], This block of code matches the code for 'mips_fpu_mode' in v8/build/toolchain.gypi. https://codereview.chromium.org/883253003/diff/20001/build/common.gypi#newcod... build/common.gypi:4075: }], This block of code matches the code for mips_dsp_rev in third_party/webrtc/build/common.gypi. https://codereview.chromium.org/883253003/diff/20001/build/common.gypi#newcod... build/common.gypi:4078: }], This block of code matches the code for mips_fpu in third_party/webrtc/build/common.gypi.
kjellander@chromium.org changed reviewers: + kjellander@chromium.org
https://codereview.chromium.org/883253003/diff/20001/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/883253003/diff/20001/build/common.gypi#newcod... build/common.gypi:4045: ['android_webview_build==0 and mips_arch_variant=="r6"', { I suggest having a single, outer, condition block with: android_webview_build==0 and then additional conditions blocks inside it to improve readability (and make it possible to use else-clauses). Applies to all the cnoditions below with android_webview_build==0 duplicated. https://codereview.chromium.org/883253003/diff/20001/build/common.gypi#newcod... build/common.gypi:4056: 'cflags': ['-mfp32'], at https://code.google.com/p/webrtc/source/browse/trunk/webrtc/build/common.gypi... the -mips32r2 flag is added to cflags+cflags_cc for this case. Shouldn't they be here too, or does -mfp32 have the same effect? https://codereview.chromium.org/883253003/diff/20001/build/common.gypi#newcod... build/common.gypi:4071: 'cflags': ['-mdsp'], https://code.google.com/p/webrtc/source/browse/trunk/webrtc/build/common.gypi... also adds this to cflags_cc. Is that redundant? https://codereview.chromium.org/883253003/diff/20001/build/common.gypi#newcod... build/common.gypi:4074: 'cflags': ['-mdspr2'], https://code.google.com/p/webrtc/source/browse/trunk/webrtc/build/common.gypi... also adds this to cflags_cc. Is that redundant? https://codereview.chromium.org/883253003/diff/20001/build/config/compiler/BU... File build/config/compiler/BUILD.gn (right): https://codereview.chromium.org/883253003/diff/20001/build/config/compiler/BU... build/config/compiler/BUILD.gn:306: cflags += [ "-mdsp" ] Similar to common.gypi: what about the cflags_cc entries? https://codereview.chromium.org/883253003/diff/20001/build/config/mips.gni File build/config/mips.gni (right): https://codereview.chromium.org/883253003/diff/20001/build/config/mips.gni#ne... build/config/mips.gni:8: mips_arch_variant = "r1" It would be very useful if you could document something about these different values as well. https://codereview.chromium.org/883253003/diff/20001/build/config/mips.gni#ne... build/config/mips.gni:24: # MIPS DSP ASE revision. Possible values are: move there three variables into the already existing declare_args block, at line 8.
https://codereview.chromium.org/883253003/diff/20001/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/883253003/diff/20001/build/common.gypi#newcod... build/common.gypi:4056: 'cflags': ['-mfp32'], cflags in GN and GYP apply to both C and C++ files. cflags_c and cflags_cc are applied on top of those to the corresponding specific types. So maybe the handling of -mips32r2 is wrong. https://codereview.chromium.org/883253003/diff/20001/build/config/mips.gni File build/config/mips.gni (right): https://codereview.chromium.org/883253003/diff/20001/build/config/mips.gni#ne... build/config/mips.gni:28: # Default: 0. You don't have to write the default in the comment. The next line lists the default so people reading the code will know, and GN will tell you the default in "gn args --list"
Brett, Henrik: thank you for the review! Please review patch set 4. I made all of the changes you suggested except one (in build/config/mips.gni). I welcome your opinions. Thanks. https://codereview.chromium.org/883253003/diff/20001/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/883253003/diff/20001/build/common.gypi#newcod... build/common.gypi:4045: ['android_webview_build==0 and mips_arch_variant=="r6"', { On 2015/02/23 13:22:35, kjellander wrote: > I suggest having a single, outer, condition block with: > android_webview_build==0 > and then additional conditions blocks inside it to improve readability Done. Good suggestion. > (and make it possible to use else-clauses). I used an else-clause for the mips_arch_variant!="r6" block. I'm not sure if it's more readable because now the code that sets the -m<hard|soft>-float flag is mixed with the code that sets the mips32 revision flag. Which form do you prefer? https://codereview.chromium.org/883253003/diff/20001/build/common.gypi#newcod... build/common.gypi:4056: 'cflags': ['-mfp32'], On 2015/02/23 18:51:36, brettw wrote: > cflags in GN and GYP apply to both C and C++ files. cflags_c and cflags_cc are > applied on top of those to the corresponding specific types. > > So maybe the handling of -mips32r2 is wrong. Brett, thank you for answering Henrik's question. webrtc/build/common.gypi incorrectly adds the same compiler flag to both cflags and cflags_cc. I verified that when compiling C++ files, that flag appears twice on the compiler command line. Henrik, this also answers your questions about the -mdsp and -mdspr2 below. https://codereview.chromium.org/883253003/diff/20001/build/config/compiler/BU... File build/config/compiler/BUILD.gn (right): https://codereview.chromium.org/883253003/diff/20001/build/config/compiler/BU... build/config/compiler/BUILD.gn:306: cflags += [ "-mdsp" ] On 2015/02/23 13:22:35, kjellander wrote: > Similar to common.gypi: what about the cflags_cc entries? Please see Brett's and my comments in build/common.gypi about cflags, cflags_c, and cflags_cc. https://codereview.chromium.org/883253003/diff/20001/build/config/mips.gni File build/config/mips.gni (right): https://codereview.chromium.org/883253003/diff/20001/build/config/mips.gni#ne... build/config/mips.gni:8: mips_arch_variant = "r1" On 2015/02/23 13:22:35, kjellander wrote: > It would be very useful if you could document something about these different > values as well. Done. https://codereview.chromium.org/883253003/diff/20001/build/config/mips.gni#ne... build/config/mips.gni:24: # MIPS DSP ASE revision. Possible values are: On 2015/02/23 13:22:35, kjellander wrote: > move there three variables into the already existing declare_args block, at line > 8. I intend to dedicate the if-else-if block on lines 5-20 to the mips_arch_variant build variable because that build variable has different default values in various cases. Please let me know if you prefer that I move the other build variable declarations to that if-else-if block. I am very open-minded about this, so please feel free to tell me! https://codereview.chromium.org/883253003/diff/20001/build/config/mips.gni#ne... build/config/mips.gni:28: # Default: 0. On 2015/02/23 18:51:36, brettw wrote: > You don't have to write the default in the comment. The next line lists the > default so people reading the code will know, and GN will tell you the default > in "gn args --list" Done. https://codereview.chromium.org/883253003/diff/60001/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/883253003/diff/60001/build/common.gypi#newcod... build/common.gypi:4055: 'cflags': ['-m<(mips_float_abi)-float'], Here I imitate the arm_float_abi code and use variable substitution. Would you prefer that I use a conditions block and spell out '-mhard-float' and '-msoft-float' in the two cases? I think it will allow people to search for these flags by name.
https://codereview.chromium.org/883253003/diff/60001/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/883253003/diff/60001/build/common.gypi#newcod... build/common.gypi:2293: ['target_arch=="mipsel" and mips_arch_variant!="r6" and android_webview_build==0', { 'hard' should be default mips_float_abi, we should not check for mips_arch_variant here. Further, this change could/should be applicable for MIPS64 as well, though MIPS64 part does not necessary need to be included in this change (we can add it later). Any webRTC specific changes should go into webRTC gypi/gni files. https://codereview.chromium.org/883253003/diff/60001/build/common.gypi#newcod... build/common.gypi:4055: 'cflags': ['-m<(mips_float_abi)-float'], mips_float_abi should not be interlinked with mips_arch_variant
Petar: thank you for the review. I made the changes you suggested in patch set 5. I also have some questions for you. https://codereview.chromium.org/883253003/diff/60001/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/883253003/diff/60001/build/common.gypi#newcod... build/common.gypi:2290: ['target_arch=="mipsel" and mips_arch_variant=="r2" and android_webview_build==0', { Petar: for mips_fpu_mode, is it correct to check for mips_arch_variant=="r2"? https://codereview.chromium.org/883253003/diff/60001/build/common.gypi#newcod... build/common.gypi:2293: ['target_arch=="mipsel" and mips_arch_variant!="r6" and android_webview_build==0', { On 2015/02/24 18:49:14, petarj wrote: > 'hard' should be default mips_float_abi, we should not check for > mips_arch_variant here. Done. Could you please take a look at the companion WebRTC CL and see if the mips_arch_variant!="r6" checks in it should be removed? https://codereview.chromium.org/883253003/diff/60001/build/common.gypi#newcod... build/common.gypi:4055: 'cflags': ['-m<(mips_float_abi)-float'], On 2015/02/24 18:49:14, petarj wrote: > mips_float_abi should not be interlinked with mips_arch_variant Done.
Thanks for taking the time to reduce the complexity on this, already complex, piece of GYP/GN code. lgtm with two possible improvements (see comments). https://codereview.chromium.org/883253003/diff/20001/build/config/mips.gni File build/config/mips.gni (right): https://codereview.chromium.org/883253003/diff/20001/build/config/mips.gni#ne... build/config/mips.gni:24: # MIPS DSP ASE revision. Possible values are: On 2015/02/23 20:15:51, wtc wrote: > > On 2015/02/23 13:22:35, kjellander wrote: > > move there three variables into the already existing declare_args block, at > line > > 8. > > I intend to dedicate the if-else-if block on lines 5-20 to the mips_arch_variant > build variable because that build variable has different default values in > various cases. > > Please let me know if you prefer that I move the other build variable > declarations to that if-else-if block. I am very open-minded about this, so > please feel free to tell me! I personally prefer not duplicating code, so I would move this up, but I'll leave it to you (or another reviewer) to decide. https://codereview.chromium.org/883253003/diff/80001/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/883253003/diff/80001/build/common.gypi#newcod... build/common.gypi:4057: ['mips_fpu_mode=="fp32"', { Would it be safe to replace line 4056-4066 with just: cflags': ['-m<(mips_fpu_mode)'], or can mips_fpu_mode have another value than these three cases? https://codereview.chromium.org/883253003/diff/80001/build/config/compiler/BU... File build/config/compiler/BUILD.gn (right): https://codereview.chromium.org/883253003/diff/80001/build/config/compiler/BU... build/config/compiler/BUILD.gn:312: cflags += [ "-mhard-float" ] I think line 311-315 can be replaced with a single line: cflags += [ "-m$mips_float_abi-float" ] but I'll leave it to you to try it out. https://codereview.chromium.org/883253003/diff/80001/build/config/compiler/BU... build/config/compiler/BUILD.gn:318: if (mips_fpu_mode == "fp32") { I think line 318-324 could be replaced with a single line: cflags += [ "-m$mips_fpu_mode" ] unless it can have another value too, which shouldn't result in a cflag getting added?
lgtm https://codereview.chromium.org/883253003/diff/60001/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/883253003/diff/60001/build/common.gypi#newcod... build/common.gypi:2290: ['target_arch=="mipsel" and mips_arch_variant=="r2" and android_webview_build==0', { On 2015/02/24 21:26:30, wtc wrote: > > Petar: for mips_fpu_mode, is it correct to check for mips_arch_variant=="r2"? > Yes, that seems correct. https://codereview.chromium.org/883253003/diff/60001/build/common.gypi#newcod... build/common.gypi:2293: ['target_arch=="mipsel" and mips_arch_variant!="r6" and android_webview_build==0', { On 2015/02/24 21:26:30, wtc wrote: > > On 2015/02/24 18:49:14, petarj wrote: > > 'hard' should be default mips_float_abi, we should not check for > > mips_arch_variant here. > > Done. > > Could you please take a look at the companion WebRTC CL and see if the > mips_arch_variant!="r6" checks in it should be removed? I did, and that part should remain in WebRTC build files, since it excludes some optimizations changes that are not applicable for MIPS32R6 variants. https://codereview.chromium.org/883253003/diff/80001/build/config/compiler/BU... File build/config/compiler/BUILD.gn (right): https://codereview.chromium.org/883253003/diff/80001/build/config/compiler/BU... build/config/compiler/BUILD.gn:318: if (mips_fpu_mode == "fp32") { On 2015/02/25 21:08:07, kjellander wrote: > I think line 318-324 could be replaced with a single line: > cflags += [ "-m$mips_fpu_mode" ] > unless it can have another value too, which shouldn't result in a cflag getting > added? > It can be undefined (if FPU unit is not present), see the case above (mips_float_abi). We could merge it with that, though I am fine with this way as well.
Please review patch set 8. I made all the changes that Henrik suggested. You can just check the delta from patch set 5, which you last reviewed. Thanks. https://codereview.chromium.org/883253003/diff/20001/build/config/mips.gni File build/config/mips.gni (right): https://codereview.chromium.org/883253003/diff/20001/build/config/mips.gni#ne... build/config/mips.gni:24: # MIPS DSP ASE revision. Possible values are: On 2015/02/25 21:08:07, kjellander wrote: > > I personally prefer not duplicating code, so I would move this up, but I'll > leave it to you (or another reviewer) to decide. Done. Please let me know if you want me to tweak the results. https://codereview.chromium.org/883253003/diff/80001/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/883253003/diff/80001/build/common.gypi#newcod... build/common.gypi:4057: ['mips_fpu_mode=="fp32"', { On 2015/02/25 21:08:07, kjellander wrote: > Would it be safe to replace line 4056-4066 with just: > cflags': ['-m<(mips_fpu_mode)'], > or can mips_fpu_mode have another value than these three cases? Done. I added an empty string check, which should make this safe. https://codereview.chromium.org/883253003/diff/80001/build/config/compiler/BU... File build/config/compiler/BUILD.gn (right): https://codereview.chromium.org/883253003/diff/80001/build/config/compiler/BU... build/config/compiler/BUILD.gn:312: cflags += [ "-mhard-float" ] On 2015/02/25 21:08:07, kjellander wrote: > I think line 311-315 can be replaced with a single line: > cflags += [ "-m$mips_float_abi-float" ] > but I'll leave it to you to try it out. Done. I added curly braces {} around the variable to improve readability. Originally I didn't want to do this because I like being able to search for a compiler flag by name. I came up with a good solution: I now document the compiler flags in the comments for mips_float_abi. https://codereview.chromium.org/883253003/diff/80001/build/config/compiler/BU... build/config/compiler/BUILD.gn:318: if (mips_fpu_mode == "fp32") { On 2015/02/26 01:35:43, petarj wrote: > > It can be undefined (if FPU unit is not present), see the case above > (mips_float_abi). We could merge it with that, though I am fine with this > way as well. I added an empty string check, so it is three lines: if (mips_fpu_mode != "") { cflags += [ "-m$mips_fpu_mode" ] }
Nice work! Now this stuff is almost readable ;) still lgtm
The CQ bit was checked by wtc@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from petarj@mips.com Link to the patchset: https://codereview.chromium.org/883253003/#ps140001 (title: "Adjust if statement nesting")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/883253003/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
wtc@chromium.org changed reviewers: - benchan@chromium.org
Brett: please review this CL. I need the approval by someone with OWNERS status for build/. Thanks.
lgtm
The CQ bit was checked by wtc@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/883253003/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/ff66006e62449071ab87ad01b025dac4a9029d06 Cr-Commit-Position: refs/heads/master@{#318524}
Message was sent while issue was closed.
Petar: I have a question for you. https://codereview.chromium.org/883253003/diff/140001/build/config/compiler/B... File build/config/compiler/BUILD.gn (right): https://codereview.chromium.org/883253003/diff/140001/build/config/compiler/B... build/config/compiler/BUILD.gn:299: cflags += [ "-m$mips_fpu_mode" ] Petar: should we use the -mfp32, -mfp64, -mfpxx flags only when mips_float_api is "hard"? If so, I can write a follow-up CL to do that. Thanks.
Message was sent while issue was closed.
https://codereview.chromium.org/883253003/diff/140001/build/config/compiler/B... File build/config/compiler/BUILD.gn (right): https://codereview.chromium.org/883253003/diff/140001/build/config/compiler/B... build/config/compiler/BUILD.gn:299: cflags += [ "-m$mips_fpu_mode" ] On 2015/02/28 00:32:14, wtc wrote: > > Petar: should we use the -mfp32, -mfp64, -mfpxx flags only when mips_float_api > is "hard"? > > If so, I can write a follow-up CL to do that. Thanks. Yes, these flags only make sense if FPU unit is used. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
