Re: [PATCH] alternative hirate for builtin_expert
Can someone comment / approve it quickly so that we get AArch32 and AArch64 linux cross-builds back up ? Ok. Applied for Dehao as r203269 . Tests on arm came back ok. Ramana Thanks, Richard. regards Ramana Honza Dehao Honza
Re: [PATCH] alternative hirate for builtin_expert
Thanks for applying the patch. Backported to google-4_8 I still have some concern when inlining .part function into its original function: basically, the gimple_block for that call may be NULL, but it does not make sense to clear all block info for all stmts in the .part function. Dehao On Tue, Oct 8, 2013 at 1:35 AM, Ramana Radhakrishnan ramana@googlemail.com wrote: Can someone comment / approve it quickly so that we get AArch32 and AArch64 linux cross-builds back up ? Ok. Applied for Dehao as r203269 . Tests on arm came back ok. Ramana Thanks, Richard. regards Ramana Honza Dehao Honza
Re: [PATCH] alternative hirate for builtin_expert
On 10/04/13 22:23, Jan Hubicka wrote: On Fri, Oct 4, 2013 at 11:54 AM, Jan Hubicka hubi...@ucw.cz wrote: I looked at this problem. Bug updated http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58619 This is a bug when updating block during tree-inline. Basically, it is legal for *n to be NULL. E.g. When gimple_block(id-gimple_call) is NULL, remap_blocks_to_null will be called to set *n to NULL. The NULL in gimple_block (gimple_call) comes from the call introduced by ipa-split? That is correct. I remember that ipa-split used to try to put the call into block since we was ICEing in similar ways previously, too. Perhaps this has changed with new BLOCK representation? The new BLOCK representation does not change this. I think it makes sense to leave the block of newly introduced call_stmt as NULL because when it's inlined back, we don't want to add additional block layers. You are right, it may be result of Jakub's changes in the area (to improve debug info after inlining back). I guess the patch makes sense then. It at-least fixes the issues I've been seeing on arm-none-linux-gnueabihf. The build has atleast gone past that point and I should have some test results later today. I don't know enough in that area to comment further on the technical aspects of the patch but it doesn't look outrageous from my point of view . Can someone comment / approve it quickly so that we get AArch32 and AArch64 linux cross-builds back up ? regards Ramana Honza Dehao Honza
Re: [PATCH] alternative hirate for builtin_expert
On Mon, Oct 7, 2013 at 12:02 PM, Ramana Radhakrishnan ramra...@arm.com wrote: On 10/04/13 22:23, Jan Hubicka wrote: On Fri, Oct 4, 2013 at 11:54 AM, Jan Hubicka hubi...@ucw.cz wrote: I looked at this problem. Bug updated http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58619 This is a bug when updating block during tree-inline. Basically, it is legal for *n to be NULL. E.g. When gimple_block(id-gimple_call) is NULL, remap_blocks_to_null will be called to set *n to NULL. The NULL in gimple_block (gimple_call) comes from the call introduced by ipa-split? That is correct. I remember that ipa-split used to try to put the call into block since we was ICEing in similar ways previously, too. Perhaps this has changed with new BLOCK representation? The new BLOCK representation does not change this. I think it makes sense to leave the block of newly introduced call_stmt as NULL because when it's inlined back, we don't want to add additional block layers. You are right, it may be result of Jakub's changes in the area (to improve debug info after inlining back). I guess the patch makes sense then. It at-least fixes the issues I've been seeing on arm-none-linux-gnueabihf. The build has atleast gone past that point and I should have some test results later today. I don't know enough in that area to comment further on the technical aspects of the patch but it doesn't look outrageous from my point of view . Can someone comment / approve it quickly so that we get AArch32 and AArch64 linux cross-builds back up ? Ok. Thanks, Richard. regards Ramana Honza Dehao Honza
Re: [PATCH] alternative hirate for builtin_expert
On 10/02/13 23:49, Rong Xu wrote: Here is the new patch. Honaz: Could you take a look? Thanks, -Rong On Wed, Oct 2, 2013 at 2:31 PM, Jan Hubicka hubi...@ucw.cz wrote: Thanks for the suggestion. This is much cleaner than to use binary parameter. Just want to make sure I understand it correctly about the orginal hitrate: you want to retire the hitrate in PRED_BUILTIN_EXPECT and always use the one specified in the biniltin-expect-probability parameter. Yes. Should I use 90% as the default? It's hard to fit current value 0.9996 in percent form. Yes, 90% seems fine. The original value was set quite arbitrarily and no real performance study was made as far as I know except yours. I think users that are sure they use expect to gueard completely cold edges may just use 100% instead of 0.9996, so I would not worry much about the precision. Honza -Rong OK with that change. Honza This broke arm-linux-gnueabihf building libstdc++-v3. I haven't dug further yet but still reducing the testcase. See http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58619 for details. Can you please deal with this appropriately ? regards Ramana
Re: [PATCH] alternative hirate for builtin_expert
Dehao, can you take a look? David On Fri, Oct 4, 2013 at 6:05 AM, Ramana Radhakrishnan ramra...@arm.com wrote: On 10/02/13 23:49, Rong Xu wrote: Here is the new patch. Honaz: Could you take a look? Thanks, -Rong On Wed, Oct 2, 2013 at 2:31 PM, Jan Hubicka hubi...@ucw.cz wrote: Thanks for the suggestion. This is much cleaner than to use binary parameter. Just want to make sure I understand it correctly about the orginal hitrate: you want to retire the hitrate in PRED_BUILTIN_EXPECT and always use the one specified in the biniltin-expect-probability parameter. Yes. Should I use 90% as the default? It's hard to fit current value 0.9996 in percent form. Yes, 90% seems fine. The original value was set quite arbitrarily and no real performance study was made as far as I know except yours. I think users that are sure they use expect to gueard completely cold edges may just use 100% instead of 0.9996, so I would not worry much about the precision. Honza -Rong OK with that change. Honza This broke arm-linux-gnueabihf building libstdc++-v3. I haven't dug further yet but still reducing the testcase. See http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58619 for details. Can you please deal with this appropriately ? regards Ramana
Re: [PATCH] alternative hirate for builtin_expert
My change on the probability of builtin_expect does have an impact on the partial inline (more outlined functions will get inline back to the original function). I think this triggers an existing issue. Dehao will explain this in his coming email. -Rong On Fri, Oct 4, 2013 at 6:05 AM, Ramana Radhakrishnan ramra...@arm.com wrote: On 10/02/13 23:49, Rong Xu wrote: Here is the new patch. Honaz: Could you take a look? Thanks, -Rong On Wed, Oct 2, 2013 at 2:31 PM, Jan Hubicka hubi...@ucw.cz wrote: Thanks for the suggestion. This is much cleaner than to use binary parameter. Just want to make sure I understand it correctly about the orginal hitrate: you want to retire the hitrate in PRED_BUILTIN_EXPECT and always use the one specified in the biniltin-expect-probability parameter. Yes. Should I use 90% as the default? It's hard to fit current value 0.9996 in percent form. Yes, 90% seems fine. The original value was set quite arbitrarily and no real performance study was made as far as I know except yours. I think users that are sure they use expect to gueard completely cold edges may just use 100% instead of 0.9996, so I would not worry much about the precision. Honza -Rong OK with that change. Honza This broke arm-linux-gnueabihf building libstdc++-v3. I haven't dug further yet but still reducing the testcase. See http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58619 for details. Can you please deal with this appropriately ? regards Ramana
Re: [PATCH] alternative hirate for builtin_expert
I looked at this problem. Bug updated http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58619 This is a bug when updating block during tree-inline. Basically, it is legal for *n to be NULL. E.g. When gimple_block(id-gimple_call) is NULL, remap_blocks_to_null will be called to set *n to NULL. The NULL in gimple_block (gimple_call) comes from the call introduced by ipa-split? I remember that ipa-split used to try to put the call into block since we was ICEing in similar ways previously, too. Perhaps this has changed with new BLOCK representation? Honza
Re: [PATCH] alternative hirate for builtin_expert
On Fri, Oct 4, 2013 at 11:54 AM, Jan Hubicka hubi...@ucw.cz wrote: I looked at this problem. Bug updated http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58619 This is a bug when updating block during tree-inline. Basically, it is legal for *n to be NULL. E.g. When gimple_block(id-gimple_call) is NULL, remap_blocks_to_null will be called to set *n to NULL. The NULL in gimple_block (gimple_call) comes from the call introduced by ipa-split? That is correct. I remember that ipa-split used to try to put the call into block since we was ICEing in similar ways previously, too. Perhaps this has changed with new BLOCK representation? The new BLOCK representation does not change this. I think it makes sense to leave the block of newly introduced call_stmt as NULL because when it's inlined back, we don't want to add additional block layers. Dehao Honza
Re: [PATCH] alternative hirate for builtin_expert
On Fri, Oct 4, 2013 at 11:54 AM, Jan Hubicka hubi...@ucw.cz wrote: I looked at this problem. Bug updated http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58619 This is a bug when updating block during tree-inline. Basically, it is legal for *n to be NULL. E.g. When gimple_block(id-gimple_call) is NULL, remap_blocks_to_null will be called to set *n to NULL. The NULL in gimple_block (gimple_call) comes from the call introduced by ipa-split? That is correct. I remember that ipa-split used to try to put the call into block since we was ICEing in similar ways previously, too. Perhaps this has changed with new BLOCK representation? The new BLOCK representation does not change this. I think it makes sense to leave the block of newly introduced call_stmt as NULL because when it's inlined back, we don't want to add additional block layers. You are right, it may be result of Jakub's changes in the area (to improve debug info after inlining back). I guess the patch makes sense then. Honza Dehao Honza
Re: [PATCH] alternative hirate for builtin_expert
Here is the new patch. Honaz: Could you take a look? OK, thanks! Honza
Re: [PATCH] alternative hirate for builtin_expert
Hi, Current default probability for builtin_expect is 0.9996. This makes the freq of unlikely bb very low (4), which suppresses the inlining of any calls within those bb. We used FDO data to measure the branch probably for the branch annotated with builtin_expert. For google internal benchmarks, the weight average (the profile count value as the weight) is 0.9081. Linux kernel is another program that is heavily annotated with builtin-expert. We measured its weight average as 0.8717, using google search as the workload. This patch sets the alternate hirate probability for builtin_expert to 90%. With the alternate hirate, we measured performance improvement for google benchmarks and Linux kernel. An earlier discussion is https://mail.google.com/mail/u/0/?pli=1#label/gcc-paches/1415c5910054630b This new patch is for the trunk and addresses Honza's comments. Honza: this new probability is off by default. When we backport to google branch we will make it the default. Let me know if you want to do the same here. I do not like much the binary parameter for builtin-expect-probability-relaxed. I would just add bulitin-expect-probability taking value in percents and then make predict.c to use it. Just use predict_edge instead of predict_edge_def and document hitrate value as unused in predict.def. OK with that change. Honza
Re: [PATCH] alternative hirate for builtin_expert
On Wed, Oct 2, 2013 at 9:08 AM, Jan Hubicka hubi...@ucw.cz wrote: Hi, Current default probability for builtin_expect is 0.9996. This makes the freq of unlikely bb very low (4), which suppresses the inlining of any calls within those bb. We used FDO data to measure the branch probably for the branch annotated with builtin_expert. For google internal benchmarks, the weight average (the profile count value as the weight) is 0.9081. Linux kernel is another program that is heavily annotated with builtin-expert. We measured its weight average as 0.8717, using google search as the workload. This patch sets the alternate hirate probability for builtin_expert to 90%. With the alternate hirate, we measured performance improvement for google benchmarks and Linux kernel. An earlier discussion is https://mail.google.com/mail/u/0/?pli=1#label/gcc-paches/1415c5910054630b This new patch is for the trunk and addresses Honza's comments. Honza: this new probability is off by default. When we backport to google branch we will make it the default. Let me know if you want to do the same here. I do not like much the binary parameter for builtin-expect-probability-relaxed. I would just add bulitin-expect-probability taking value in percents and then make predict.c to use it. Just use predict_edge instead of predict_edge_def and document hitrate value as unused in predict.def. Thanks for the suggestion. This is much cleaner than to use binary parameter. Just want to make sure I understand it correctly about the orginal hitrate: you want to retire the hitrate in PRED_BUILTIN_EXPECT and always use the one specified in the biniltin-expect-probability parameter. Should I use 90% as the default? It's hard to fit current value 0.9996 in percent form. -Rong OK with that change. Honza
Re: [PATCH] alternative hirate for builtin_expert
Thanks for the suggestion. This is much cleaner than to use binary parameter. Just want to make sure I understand it correctly about the orginal hitrate: you want to retire the hitrate in PRED_BUILTIN_EXPECT and always use the one specified in the biniltin-expect-probability parameter. Yes. Should I use 90% as the default? It's hard to fit current value 0.9996 in percent form. Yes, 90% seems fine. The original value was set quite arbitrarily and no real performance study was made as far as I know except yours. I think users that are sure they use expect to gueard completely cold edges may just use 100% instead of 0.9996, so I would not worry much about the precision. Honza -Rong OK with that change. Honza
Re: [PATCH] alternative hirate for builtin_expert
Here is the new patch. Honaz: Could you take a look? Thanks, -Rong On Wed, Oct 2, 2013 at 2:31 PM, Jan Hubicka hubi...@ucw.cz wrote: Thanks for the suggestion. This is much cleaner than to use binary parameter. Just want to make sure I understand it correctly about the orginal hitrate: you want to retire the hitrate in PRED_BUILTIN_EXPECT and always use the one specified in the biniltin-expect-probability parameter. Yes. Should I use 90% as the default? It's hard to fit current value 0.9996 in percent form. Yes, 90% seems fine. The original value was set quite arbitrarily and no real performance study was made as far as I know except yours. I think users that are sure they use expect to gueard completely cold edges may just use 100% instead of 0.9996, so I would not worry much about the precision. Honza -Rong OK with that change. Honza p2_patch_v2 Description: Binary data
Re: [PATCH] alternative hirate for builtin_expert
ping. On Fri, Sep 27, 2013 at 3:53 PM, Rong Xu x...@google.com wrote: Hi, Current default probability for builtin_expect is 0.9996. This makes the freq of unlikely bb very low (4), which suppresses the inlining of any calls within those bb. We used FDO data to measure the branch probably for the branch annotated with builtin_expert. For google internal benchmarks, the weight average (the profile count value as the weight) is 0.9081. Linux kernel is another program that is heavily annotated with builtin-expert. We measured its weight average as 0.8717, using google search as the workload. This patch sets the alternate hirate probability for builtin_expert to 90%. With the alternate hirate, we measured performance improvement for google benchmarks and Linux kernel. An earlier discussion is https://mail.google.com/mail/u/0/?pli=1#label/gcc-paches/1415c5910054630b This new patch is for the trunk and addresses Honza's comments. Honza: this new probability is off by default. When we backport to google branch we will make it the default. Let me know if you want to do the same here. Thanks, -Rong
[PATCH] alternative hirate for builtin_expert
Hi, Current default probability for builtin_expect is 0.9996. This makes the freq of unlikely bb very low (4), which suppresses the inlining of any calls within those bb. We used FDO data to measure the branch probably for the branch annotated with builtin_expert. For google internal benchmarks, the weight average (the profile count value as the weight) is 0.9081. Linux kernel is another program that is heavily annotated with builtin-expert. We measured its weight average as 0.8717, using google search as the workload. This patch sets the alternate hirate probability for builtin_expert to 90%. With the alternate hirate, we measured performance improvement for google benchmarks and Linux kernel. An earlier discussion is https://mail.google.com/mail/u/0/?pli=1#label/gcc-paches/1415c5910054630b This new patch is for the trunk and addresses Honza's comments. Honza: this new probability is off by default. When we backport to google branch we will make it the default. Let me know if you want to do the same here. Thanks, -Rong p2_patch Description: Binary data
