Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
843563002:
Add compiler and linker flags for MIPS. (Closed)
DescriptionAdd compiler and linker flags for MIPS.
I ported the MIPS code from build/common.gypi except for the
mips_arch_variant variable.
R=benchan@chromium.org,brettw@chromium.org,petarj@mips.com
BUG=446234
Committed: https://crrev.com/0055a3a72acc45fb9aafcc965dd77db6cc99da6d
Cr-Commit-Position: refs/heads/master@{#312480}
Patch Set 1 #Patch Set 2 : Check is_android_webview_build #
Total comments: 5
Patch Set 3 : Support mips_arch_variant. Remove unneeded flags. #
Total comments: 2
Patch Set 4 : Add build/config/mips.gni #
Total comments: 8
Patch Set 5 : Rebase #Patch Set 6 : Use else if #Messages
Total messages: 19 (2 generated)
Please review. Note that mips_arch_variant support still
needs to be added.
The equivalent code in build/common.gypi is reproduced here
for your convenience:
['target_arch=="mipsel"', {
'target_conditions': [
['_toolset=="target"', {
'conditions': [
['android_webview_build==0 and mips_arch_variant=="r6"', {
'cflags': ['-mips32r6', '-Wa,-mips32r6'],
'conditions': [
['OS=="android"', {
'ldflags': ['-mips32r6', '-Wl,-melf32ltsmip',],
}],
],
}],
['android_webview_build==0 and mips_arch_variant=="r2"', {
'cflags': ['-mips32r2', '-Wa,-mips32r2'],
}],
['android_webview_build==0 and mips_arch_variant=="r1"', {
'cflags': ['-mips32', '-Wa,-mips32'],
}],
],
'ldflags': [
'-Wl,--no-keep-memory'
],
'cflags_cc': [
'-Wno-uninitialized',
],
}],
],
}],
['target_arch=="mips64el"', {
'target_conditions': [
['_toolset=="target"', {
'conditions': [
['android_webview_build==0 and mips_arch_variant=="r6"', {
'cflags': ['-mips64r6', '-Wa,-mips64r6'],
'ldflags': [ '-mips64r6' ],
}],
['android_webview_build==0 and mips_arch_variant=="r2"', {
'cflags': ['-mips64r2', '-Wa,-mips64r2'],
'ldflags': [ '-mips64r2' ],
}],
],
'cflags_cc': [
'-Wno-uninitialized',
],
}],
],
}],
https://codereview.chromium.org/843563002/diff/20001/build/config/compiler/BU...
File build/config/compiler/BUILD.gn (right):
https://codereview.chromium.org/843563002/diff/20001/build/config/compiler/BU...
build/config/compiler/BUILD.gn:276: # from the Android build system.
I copied this comment from the cpu_arch == "arm" case on lines 242-243 above.
https://codereview.chromium.org/843563002/diff/20001/build/config/compiler/BU... File build/config/compiler/BUILD.gn (right): https://codereview.chromium.org/843563002/diff/20001/build/config/compiler/BU... build/config/compiler/BUILD.gn:278: # TODO(wtc): check mips_arch_variant (possible values are "r1", "r2", should it be done in the same CL? https://codereview.chromium.org/843563002/diff/20001/build/config/compiler/BU... build/config/compiler/BUILD.gn:286: cflags_cc += [ "-Wno-uninitialized" ] are these ldflags and cflags_cc really needed?
https://codereview.chromium.org/843563002/diff/20001/build/config/compiler/BU... File build/config/compiler/BUILD.gn (right): https://codereview.chromium.org/843563002/diff/20001/build/config/compiler/BU... build/config/compiler/BUILD.gn:278: # TODO(wtc): check mips_arch_variant (possible values are "r1", "r2", On 2015/01/07 21:48:40, Ben Chan wrote: > should it be done in the same CL? Done. https://codereview.chromium.org/843563002/diff/20001/build/config/compiler/BU... build/config/compiler/BUILD.gn:286: cflags_cc += [ "-Wno-uninitialized" ] On 2015/01/07 21:48:40, Ben Chan wrote: > are these ldflags and cflags_cc really needed? I ported the GYP code faithfully without thinking. Now that you asked, I think -Wno-uninitialized is actually harmful. I will remove it from this CL and probably from build/common.gypi as well. -Wl,--no-keep-memory seems to be a linker flag that affects the performance of ld: --no-keep-memory ld normally optimizes for speed over memory usage by caching the symbol tables of input files in memory. This option tells ld to instead optimize for memory usage, by rereading the symbol tables as necessary. This may be required if ld runs out of memory space while linking a large executable. I'll remove it until we see ld run out of memory. https://codereview.chromium.org/843563002/diff/40001/build/config/compiler/BU... File build/config/compiler/BUILD.gn (right): https://codereview.chromium.org/843563002/diff/40001/build/config/compiler/BU... build/config/compiler/BUILD.gn:40: mips_arch_variant = mips_arch_variant_default Is there a better way to provide a platform-dependent default value for a gn buildvariable? https://codereview.chromium.org/843563002/diff/40001/build/config/compiler/BU... build/config/compiler/BUILD.gn:302: "-Wl,-melf32ltsmip", I don't know what this flag is for. I copied it from build/common.gypi.
Please review patch set 3. I need the new approach to fix webrtc build. https://codereview.chromium.org/843563002/diff/60001/build/config/compiler/BU... File build/config/compiler/BUILD.gn (right): https://codereview.chromium.org/843563002/diff/60001/build/config/compiler/BU... build/config/compiler/BUILD.gn:10: import("//build/config/mips.gni") I decided to add build/config/mips.gni and declare the buildvariable mips_arch_variant there because I found third_party/webrtc already declares that buildvariable. It is best to declare that variable in a .gni file. Apparently webrtc doesn't import build/config/compiler/BUILD.gn. The companion webrtc CL is https://webrtc-codereview.appspot.com/41399004/
Ah, sorry, I meant please review patch set 4.
https://codereview.chromium.org/843563002/diff/60001/build/config/mips.gni File build/config/mips.gni (right): https://codereview.chromium.org/843563002/diff/60001/build/config/mips.gni#ne... build/config/mips.gni:7: declare_args() { Will anybody actually need to set the arch variant manually from outside the build? Most of the things we have parameterized this way are never used. I've been taking the approach of just declaring regular variables (no declare_args) and if somebody needs to set it they can come along and add declare_args.
https://codereview.chromium.org/843563002/diff/60001/build/config/mips.gni File build/config/mips.gni (right): https://codereview.chromium.org/843563002/diff/60001/build/config/mips.gni#ne... build/config/mips.gni:7: declare_args() { Henrik: does the WebRTC team need to set mips_arch_variant manually from outside the build?
kjellander@chromium.org changed reviewers: + ajm@chromium.org
On 2015/01/12 22:53:49, wtc wrote: > https://codereview.chromium.org/843563002/diff/60001/build/config/mips.gni > File build/config/mips.gni (right): > > https://codereview.chromium.org/843563002/diff/60001/build/config/mips.gni#ne... > build/config/mips.gni:7: declare_args() { > > Henrik: does the WebRTC team need to set mips_arch_variant manually from outside > the build? +ajm Andrew: can you answer this? I really don't know how and when we build for MIPS.
lgtm https://codereview.chromium.org/843563002/diff/60001/build/config/mips.gni File build/config/mips.gni (right): https://codereview.chromium.org/843563002/diff/60001/build/config/mips.gni#ne... build/config/mips.gni:7: declare_args() { On 2015/01/12 07:27:02, brettw wrote: > Will anybody actually need to set the arch variant manually from outside the > build? Most of the things we have parameterized this way are never used. I've > been taking the approach of just declaring regular variables (no declare_args) > and if somebody needs to set it they can come along and add declare_args. > We actually use this often. Being able to build Chromium for Linux mips32r1 and mips32r2 without changing the source code is very convenient.
On 2015/01/14 06:08:00, kjellander wrote: > +ajm > Andrew: can you answer this? I really don't know how and when we build for MIPS. As petarj answered, the internal webrtc team doesn't use it, but the MIPS devs who are contributing optimizations do.
Brett: we have answered your question. Could you please take another look at the CL? Thanks.
lgtm https://codereview.chromium.org/843563002/diff/60001/build/config/mips.gni File build/config/mips.gni (right): https://codereview.chromium.org/843563002/diff/60001/build/config/mips.gni#ne... build/config/mips.gni:11: if (cpu_arch == "mips64el") { Can you make this an "else if"
https://codereview.chromium.org/843563002/diff/60001/build/config/mips.gni File build/config/mips.gni (right): https://codereview.chromium.org/843563002/diff/60001/build/config/mips.gni#ne... build/config/mips.gni:8: mips_arch_variant = "r1" this definition differs from the one in webrtc.gni, and probably causes a redefinition error.
I made the change that Brett suggested in patch set 6. https://codereview.chromium.org/843563002/diff/60001/build/config/mips.gni File build/config/mips.gni (right): https://codereview.chromium.org/843563002/diff/60001/build/config/mips.gni#ne... build/config/mips.gni:8: mips_arch_variant = "r1" On 2015/01/20 23:36:34, Ben Chan wrote: > this definition differs from the one in webrtc.gni, and probably causes a > redefinition error. Yes. This Chromium CL requires a companion WebRTC CL: https://webrtc-codereview.appspot.com/41399004/ Re: "mips32r1" vs. "r1": I decided to go with "r1" because that is the form used in the GYP build system (build/common.gypi). https://codereview.chromium.org/843563002/diff/60001/build/config/mips.gni#ne... build/config/mips.gni:11: if (cpu_arch == "mips64el") { On 2015/01/20 23:34:26, brettw wrote: > Can you make this an "else if" Done.
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/843563002/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/0055a3a72acc45fb9aafcc965dd77db6cc99da6d Cr-Commit-Position: refs/heads/master@{#312480} |
