Re: Simple change in dot dumper -- Display profile count and branch probability

2013-04-20 Thread Xinliang David Li
Thanks.   The patch is revised.

David

2013-04-20  Xinliang David Li  

* graph.c (draw_cfg_node_succ_edges): Add branch probility as label.
* cfghhooks.c (dump_bb_for_graph): Dump profile count and frquency.
* Makefile.in: New dependency.

Index: cfghooks.c
===
--- cfghooks.c  (revision 198108)
+++ cfghooks.c  (working copy)
@@ -30,6 +30,7 @@ along with GCC; see the file COPYING3.
 #include "timevar.h"
 #include "diagnostic-core.h"
 #include "cfgloop.h"
+#include "pretty-print.h"

 /* A pointer to one of the hooks containers.  */
 static struct cfg_hooks *cfg_hooks;
@@ -308,6 +309,10 @@ dump_bb_for_graph (pretty_printer *pp, b
   if (!cfg_hooks->dump_bb_for_graph)
 internal_error ("%s does not support dump_bb_for_graph",
cfg_hooks->name);
+  if (bb->count)
+pp_printf (pp, "COUNT:" HOST_WIDEST_INT_PRINT_DEC, bb->count);
+  pp_printf (pp, " FREQ:%i |", bb->frequency);
+  pp_write_text_to_stream (pp);
   cfg_hooks->dump_bb_for_graph (pp, bb);
 }

Index: graph.c
===
--- graph.c (revision 198108)
+++ graph.c (working copy)
@@ -155,11 +155,12 @@ draw_cfg_node_succ_edges (pretty_printer

   pp_printf (pp,
 "\tfn_%d_basic_block_%d:s -> fn_%d_basic_block_%d:n "
-"[style=%s,color=%s,weight=%d,constraint=%s];\n",
+"[style=%s,color=%s,weight=%d,constraint=%s,
label=\"[%i%%]\"];\n",
 funcdef_no, e->src->index,
 funcdef_no, e->dest->index,
 style, color, weight,
-(e->flags & (EDGE_FAKE | EDGE_DFS_BACK)) ? "false" : "true");
+(e->flags & (EDGE_FAKE | EDGE_DFS_BACK)) ? "false" : "true",
+e->probability * 100 / REG_BR_PROB_BASE);
 }
   pp_flush (pp);
 }
Index: Makefile.in
===
--- Makefile.in (revision 198108)
+++ Makefile.in (working copy)
@@ -3123,7 +3123,7 @@ cfg.o : cfg.c $(CONFIG_H) $(SYSTEM_H) co
$(GGC_H) $(OBSTACK_H) alloc-pool.h $(HASH_TABLE_H) $(CFGLOOP_H) $(TREE_H) \
$(BASIC_BLOCK_H)
 cfghooks.o: cfghooks.c $(CONFIG_H) $(SYSTEM_H) coretypes.h $(TM_H) $(RTL_H) \
-   $(TREE_H) $(BASIC_BLOCK_H) $(TREE_FLOW_H) $(TIMEVAR_H) toplev.h
$(DIAGNOSTIC_CORE_H) $(CFGLOOP_H)
+   $(TREE_H) $(BASIC_BLOCK_H) $(TREE_FLOW_H) $(TIMEVAR_H) toplev.h
$(DIAGNOSTIC_CORE_H) $(CFGLOOP_H) $(PRETTY_PRINT_H)
 cfgexpand.o : cfgexpand.c $(TREE_FLOW_H) $(CONFIG_H) $(SYSTEM_H) \
$(RTL_H) $(TREE_H) $(TM_P_H) $(EXPR_H) $(FUNCTION_H) $(TM_H) \
coretypes.h $(EXCEPT_H) langhooks.h $(TREE_PASS_H) $(RTL_H) \

On Sat, Apr 20, 2013 at 4:02 PM, Steven Bosscher  wrote:
> On Sun, Apr 21, 2013 at 12:47 AM, Xinliang David Li wrote:
>> Index: graph.c
>> ===
>> --- graph.c (revision 198108)
>> +++ graph.c (working copy)
>> @@ -110,6 +110,9 @@ draw_cfg_node (pretty_printer *pp, int f
>>else
>>  {
>>pp_character (pp, '{');
>> +  if (bb->count)
>> +   pp_printf (pp, "COUNT:" HOST_WIDEST_INT_PRINT_DEC, bb->count);
>> +  pp_printf (pp, " FREQ:%i |", bb->frequency);
>>pp_write_text_to_stream (pp);
>>dump_bb_for_graph (pp, bb);
>>pp_character (pp, '}');
>
> This doesn't belong here, please put it in cfghooks.c:dump_bb_for_graph.
>
> Otherwise, looks good me.
>
> Ciao!
> Steven


Re: GCC does not support *mmintrin.h with function specific opts

2013-04-20 Thread Sriraman Tallam
Ping.

On Wed, Apr 17, 2013 at 7:13 PM, Sriraman Tallam  wrote:
> +HJ
>
> On Tue, Apr 16, 2013 at 1:54 PM, Sriraman Tallam  wrote:
>> Hi,
>>
>> I have attached an updated patch that  addresses all the comments raised.
>>
>> On Fri, Apr 12, 2013 at 1:58 AM, Jakub Jelinek  wrote:
>>> On Thu, Apr 11, 2013 at 12:05:41PM -0700, Sriraman Tallam wrote:
 I have attached a patch that fixes this. I have added an option
 "-mgenerate-builtins" that will do two things.  It will define a macro
 "__ALL_ISA__" which will expose the *intrin.h functions. It will also
 expose all the target specific builtins.  -mgenerate-builtins will not
 affect code generation.
>>>
>>> 1) this shouldn't be an option, either it can be made to work reliably,
>>>then it should be done always, or it can't, then it shouldn't be done
>>
>> Ok, it is on by default now.  There is a way to turn it off, with
>> -mno-generate-builtins.
>>
>>> 2) have you verified that if you always generate all builtins, that the
>>>builtins not supported by the ISA selected from the command line are
>>>created with the right vector modes?
>>
>> This issue does not arise.  When the target builtin is expanded, it is
>> checked if the ISA support is there, either via function specific
>> target opts or global target opts. If not, an error is raised. Test
>> case added for this, please see intrinsic_4.c in patch.
>>
>>> 3) the *intrin.h headers in the case where the guarding macro isn't defined
>>>should be surrounded by something like
>>>#ifndef __FMA4__
>>>#pragma GCC push options
>>>#pragma GCC target("fma4")
>>>#endif
>>>...
>>>#ifndef __FMA4__
>>>#pragma GCC pop options
>>>#endif
>>>so that everything that is in the headers is compiled with the ISA
>>>in question
>>
>> I do not think this should be done because it will break the inlining
>> ability of the header function and cause issues if the caller does not
>> specify the required ISA. The fact that the header functions are
>> marked extern __inline, with gnu_inline guarantees that a body will
>> not be generated and they will be inlined.  If the caller does not
>> have the required ISA, appropriate errors will be raised. Test cases
>> added, see intrinsics_1.c, intrinsics_2.c
>>
>>> 4) what happens if you use the various vector types typedefed in the
>>>*intrin.h headers in code that doesn't support those ISAs?  As TYPE_MODE
>>>for VECTOR_TYPE is a function call, perhaps it will just be handled as
>>>generic BLKmode vectors, which is desirable I think
>>
>> I checked some tests here.  With -mno-sse for instance, vector types
>> are not permitted in function arguments and return values and gcc
>> raises a warning/error in each case.  With return values, gcc always
>> gives an error if a SSE register is required in a return value.  I
>> even fixed this message to not do it for functions marked as extern
>> inline, with "gnu_inline" keyword as a body for them will not be
>> generated.
>>
>>
>>> 5) what happens if you use a target builtin in a function not supporting
>>>the corresponding ISA, do you get proper error explaining what you are
>>>doing wrong?
>>
>> Yes, please sse intrinsic_4.c test in patch.
>>
>>> 6) what happens if you use some intrinsics in a function not supporting
>>>the corresponding ISA?  Dunno if the inliner chooses not to inline it
>>>and error out because it is always_inline, or what exactly will happen
>>>then
>>
>> Same deal here. The intrinsic function will, guaranteed, to be inlined
>> into the caller which will be a corresponding builtin call. That
>> builtin call will trigger an error if the ISA is not supported.
>>
>> Thanks
>> Sri
>>
>>>
>>> For all this you certainly need testcases.
>>>
>>> Jakub


Re: [google][4.7] Generate a label for the split cold function while using -freorder-blocks-and-partition

2013-04-20 Thread Sriraman Tallam
Fixed a bug in the previous patch sent, where  I did not check if the
switch section was actually to the cold function part.  Updated patch
attached.

Thanks
Sri



On Fri, Apr 19, 2013 at 1:58 PM, Sriraman Tallam  wrote:
> Updated patch attached.
>
> Thanks
> Sri
>
> On Fri, Apr 19, 2013 at 1:43 PM, Sriraman Tallam  wrote:
>> Hi,
>>
>>   This patch generates labels for cold function parts that are split when
>> using the option -freorder-blocks-and-partition.  The cold label name
>> is generated by suffixing ".cold" to the assembler name of the hot
>> function.
>>
>>   This is useful when getting back traces from gdb when the cold function
>> part does get executed.
>>
>> I will port this patch to trunk, please let me know what you think.
>>
>> Thanks
>> Sri
Patch to generate labels for cold function parts that are split when
using the option -freorder-blocks-and-partition.  The cold label name
is generated by suffixing ".cold" to the assembler name of the hot
function.

This is useful when getting back traces from gdb when the cold function
part does get executed. 

Index: final.c
===
--- final.c (revision 198081)
+++ final.c (working copy)
@@ -1934,6 +1934,21 @@ final_scan_insn (rtx insn, FILE *file, int optimiz
  targetm.asm_out.function_switched_text_sections (asm_out_file,
   
current_function_decl,
   in_cold_section_p);
+ /* Emit a label for the split cold section.  Form label name by
+suffixing ".cold" to the function's assembler name.  */
+ if (in_cold_section_p)
+   {
+ char *cold_function_name;
+ const char *mangled_function_name;
+ tree asm_name = DECL_ASSEMBLER_NAME (current_function_decl);
+
+ mangled_function_name = IDENTIFIER_POINTER (asm_name);
+ cold_function_name = XNEWVEC (char,
+ strlen (mangled_function_name) + strlen (".cold") + 1);
+ sprintf (cold_function_name, "%s.cold", mangled_function_name);
+ ASM_OUTPUT_LABEL (asm_out_file, cold_function_name);
+ XDELETEVEC (cold_function_name);
+   }
  break;
 
case NOTE_INSN_BASIC_BLOCK:
Index: testsuite/gcc.dg/tree-prof/cold_partition_label.c
===
--- testsuite/gcc.dg/tree-prof/cold_partition_label.c   (revision 0)
+++ testsuite/gcc.dg/tree-prof/cold_partition_label.c   (revision 0)
@@ -0,0 +1,39 @@
+/* Test case to check if function foo gets split and the cold function
+   gets a label.  */
+/* { dg-require-effective-target freorder } */
+/* { dg-options "-O2 -freorder-blocks-and-partition --save-temps" } */
+
+#define SIZE 1
+
+const char *sarr[SIZE];
+const char *buf_hot;
+const char *buf_cold;
+
+__attribute__((noinline))
+void 
+foo (int path)
+{
+  int i;
+  if (path)
+{
+  for (i = 0; i < SIZE; i++)
+   sarr[i] = buf_hot;
+}
+  else
+{
+  for (i = 0; i < SIZE; i++)
+   sarr[i] = buf_cold;
+}
+}
+
+int
+main (int argc, char *argv[])
+{
+  buf_hot =  "hello";
+  buf_cold = "world";
+  foo (argc);
+  return 0;
+}
+
+/* { dg-final-use { scan-assembler "foo.cold" } } */
+/* { dg-final-use { cleanup-saved-temps } } */


Re: Simple change in dot dumper -- Display profile count and branch probability

2013-04-20 Thread Steven Bosscher
On Sun, Apr 21, 2013 at 12:47 AM, Xinliang David Li wrote:
> Index: graph.c
> ===
> --- graph.c (revision 198108)
> +++ graph.c (working copy)
> @@ -110,6 +110,9 @@ draw_cfg_node (pretty_printer *pp, int f
>else
>  {
>pp_character (pp, '{');
> +  if (bb->count)
> +   pp_printf (pp, "COUNT:" HOST_WIDEST_INT_PRINT_DEC, bb->count);
> +  pp_printf (pp, " FREQ:%i |", bb->frequency);
>pp_write_text_to_stream (pp);
>dump_bb_for_graph (pp, bb);
>pp_character (pp, '}');

This doesn't belong here, please put it in cfghooks.c:dump_bb_for_graph.

Otherwise, looks good me.

Ciao!
Steven


Simple change in dot dumper -- Display profile count and branch probability

2013-04-20 Thread Xinliang David Li
Hi, the graph dump file currently does not show any profile
information. The following simple patch fixed that.  Ok for trunk?

thanks,

David

2013-04-20  Xinliang David Li  

* graph.c (draw_cfg_node): Add count and frequency info.
(draw_cfg_node_succ_edges): Add branch probility as label.

Index: graph.c
===
--- graph.c (revision 198108)
+++ graph.c (working copy)
@@ -110,6 +110,9 @@ draw_cfg_node (pretty_printer *pp, int f
   else
 {
   pp_character (pp, '{');
+  if (bb->count)
+   pp_printf (pp, "COUNT:" HOST_WIDEST_INT_PRINT_DEC, bb->count);
+  pp_printf (pp, " FREQ:%i |", bb->frequency);
   pp_write_text_to_stream (pp);
   dump_bb_for_graph (pp, bb);
   pp_character (pp, '}');
@@ -155,11 +158,12 @@ draw_cfg_node_succ_edges (pretty_printer

   pp_printf (pp,
 "\tfn_%d_basic_block_%d:s -> fn_%d_basic_block_%d:n "
-"[style=%s,color=%s,weight=%d,constraint=%s];\n",
+"[style=%s,color=%s,weight=%d,constraint=%s,
label=\"[%i%%]\"];\n",
 funcdef_no, e->src->index,
 funcdef_no, e->dest->index,
 style, color, weight,
-(e->flags & (EDGE_FAKE | EDGE_DFS_BACK)) ? "false" : "true");
+(e->flags & (EDGE_FAKE | EDGE_DFS_BACK)) ? "false" : "true",
+e->probability * 100 / REG_BR_PROB_BASE);
 }
   pp_flush (pp);
 }


Re: [PATCH] Fix PR56982, handle setjmp like non-local labels

2013-04-20 Thread Ian Lance Taylor
On Sat, Apr 20, 2013 at 5:14 AM, Andreas Schwab  wrote:
> Richard Biener  writes:
>
>>   PR tree-optimization/56982
>>   * builtins.def (BUILT_IN_LONGJMP): longjmp is not a leaf
>>   function.
>>   * gimplify.c (gimplify_call_expr): Notice special calls.
>>   (gimplify_modify_expr): Likewise.
>>   * tree-cfg.c (make_abnormal_goto_edges): Handle setjmp-like
>>   abnormal control flow receivers.
>>   (call_can_make_abnormal_goto): Handle cfun->calls_setjmp
>>   in the same way as cfun->has_nonlocal_labels.
>>   (gimple_purge_dead_abnormal_call_edges): Likewise.
>>   (stmt_starts_bb_p): Make setjmp-like abnormal control flow
>>   receivers start a basic-block.
>
> This breaks libgo.
>
> ../../../gcc/libgo/runtime/proc.c: In function ‘runtime_mcall.constprop.7’:
> ../../../gcc/libgo/runtime/proc.c:419:13: error: ‘({anonymous})’ may be used 
> uninitialized in this function [-Werror=maybe-uninitialized]
>getcontext(&gp->context);
>  ^

That's an unusually useless error message.  There are no local structs
in this function.  Without digging into the compiler I'm not sure what
this means or how to fix it.

Ian


[patch] fix anachronism in libstdc+ docs

2013-04-20 Thread Jonathan Wakely
* doc/xml/manual/extensions.xml: Fix anachronism.

committed to trunk
commit ab11f53f4046adc88e1f43568836c572bcd3bc44
Author: Jonathan Wakely 
Date:   Sat Apr 20 20:31:32 2013 +0100

* doc/xml/manual/extensions.xml: Fix anachronism.

diff --git a/libstdc++-v3/doc/xml/manual/extensions.xml 
b/libstdc++-v3/doc/xml/manual/extensions.xml
index 1f3da2f..7b35d05 100644
--- a/libstdc++-v3/doc/xml/manual/extensions.xml
+++ b/libstdc++-v3/doc/xml/manual/extensions.xml
@@ -87,10 +87,11 @@ extensions, be aware of two things:
   3.1, 3.2 and 3.3).

 
-   Please note that the upcoming C++ standard has first-class
+   Please note that the concept checks only validate the requirements
+   of the old C++03 standard. C++11 was expected to have first-class
support for template parameter constraints based on concepts in the core
-   language. This will obviate the need for the library-simulated concept
-   checking described above.
+   language. This would have obviated the need for the library-simulated 
concept
+   checking described above, but was not part of C++11.

 
 


Re: [patch, fortran, backport, 4.8] PR51825 - Fortran runtime error: Cannot match namelist object name

2013-04-20 Thread Tilo Schwarz

On Sat, 13 Apr 2013 10:29:26 +0200, Janus Weil  wrote:


Hi Tilo,


I would like to backport the fix for PR51825 I posted here

http://gcc.gnu.org/ml/gcc-patches/2013-03/msg00316.html

to the 4.8 branch.


well, the usual gfortran policy is to only do backports of regression
fixes. In exceptional cases, also non-regression fixes can be
backported (and have been in the past), if the bug is extremely severe
and/or the the fix is extremely simple (IMHO the most severe type of
bug is a wrong-code issue, where the user does not see any kind of
error message, but just 'silently' gets wrong results).


Hi Janus,

the bug is indeed pretty nasty. What happens is, that any namelist line  
involving an array index and nested derived types like this


arr(2)%a%b = 1

completely ignores the given array index and silently reads it as 1.

So no matter what you write as index > 1 in a line like

arr()%a%b = 1

libgfortran always silently reads the index == 1 as

arr(1)%a%b = 1

and sets the first element of arr instead of setting arr(index).

When I hit this (as a gfortran user) it took me quite a while to figure  
out whats going on. What makes it even more nasty: This


arr(2)%b = 1

did work (only one level of derived type given).

I'm personally not too affected anymore, because this bug made me writing  
my first libgfortran patch ever and since then I use gfortran trunk anyway  
;-)


But for others the bug is "sort of mean" ...


Regards,

Tilo


Re: [PATCH] Fix PR56982, handle setjmp like non-local labels

2013-04-20 Thread Andreas Schwab
Richard Biener  writes:

>   PR tree-optimization/56982
>   * builtins.def (BUILT_IN_LONGJMP): longjmp is not a leaf
>   function.
>   * gimplify.c (gimplify_call_expr): Notice special calls.
>   (gimplify_modify_expr): Likewise.
>   * tree-cfg.c (make_abnormal_goto_edges): Handle setjmp-like
>   abnormal control flow receivers.
>   (call_can_make_abnormal_goto): Handle cfun->calls_setjmp
>   in the same way as cfun->has_nonlocal_labels.
>   (gimple_purge_dead_abnormal_call_edges): Likewise.
>   (stmt_starts_bb_p): Make setjmp-like abnormal control flow
>   receivers start a basic-block.

This breaks libgo.

../../../gcc/libgo/runtime/proc.c: In function ‘runtime_mcall.constprop.7’:
../../../gcc/libgo/runtime/proc.c:419:13: error: ‘({anonymous})’ may be used 
uninitialized in this function [-Werror=maybe-uninitialized]
   getcontext(&gp->context);
 ^

Andreas.

-- 
Andreas Schwab, sch...@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."


Re: [patch][mips] split mips_reorg in pre- and post-dbr_schedule parts

2013-04-20 Thread Steven Bosscher
*ping* MIPS maintainers...

On Mon, Apr 15, 2013 at 5:09 PM, Jeff Law wrote:
> On 04/14/2013 08:20 AM, Steven Bosscher wrote:
>>
>> Hello,
>>
>> This patch splits mips_reorg.c in a pre-dbr_schedule part and a new,
>> machine specific post-dbr_schedule pass. With this patch,
>> cleanup_barriers and dbr_schedule can be static functions again.
>>
>> Cross-built&tested mips-sim. OK for trunk?
>>
>> Ciao!
>> Steven
>>
>>
>> mips_post_dbr_reorg_as_machine_pass.diff.txt
>>
>>
>> * config/mips/mips.c: Include tree-pass.h.
>> (mips_reorg): Split in pre- and post-dbr_schedule parts.
>> (mips_machine_reorg2): Move mips_reorg post-dbr_schedule parts
>> here.
>> (pass_mips_machine_reorg2): New machine specific pass.
>> (insert_pass_mips_machine_reorg2): New pass plugin definition.
>> (mips_option_override): Register the new pass.
>> * rtl.h (cleanup_barriers): Remove prototype.
>> (dbr_schedule): Likewise.
>> * jump.c (cleanup_barriers): Make static.
>> * reorg.c (dbr_schedule): Likewise.
>
> The rtl, jump & reorg bits are fine with me.  I don't know enough about the
> MIPS specific bits to comment on them in any meaningful way.
>
> jeff
>


Re: [Patch, Fortran] PR56907 - do not 'pack' arrays passed to C_LOC

2013-04-20 Thread Janus Weil
>>> Well, it does: As it doesn't know whether the array is contiguous or not
>>> - it packs the array. That's what one usually wants. However, for C_LOC one
>>> knows that the array is contiguous - the user promises this to the compiler
>>> - and, hence, no packing is needed.
>>
>> Ok, I see. Then the patch is certainly ok. (The fact that
>> conv_isocbinding_function is used also for C_ASSOCIATED and C_FUNLOC
>> is no problem either, I guess).
>
> Well, the code in question is under:
>
>   if (expr->value.function.isym->id == GFC_ISYM_C_LOC)

Ah, sure, I missed that.

Btw, wouldn't it make more sense to split up
'conv_isocbinding_function' into three separate functions, since there
isn't really any common code and one could directly call them in:

case GFC_ISYM_C_ASSOCIATED:
case GFC_ISYM_C_FUNLOC:
case GFC_ISYM_C_LOC:
  conv_isocbinding_function (se, expr);
  break;


Anyway, the patch is ok as is (or with this additional cleanup, if you prefer).


>> However, if calling C_LOC on a non-contiguous array is invalid,
>> shouldn't one add a check for cases like
>>integer, dimension(1:5,1:5), target :: zzz
>>type(c_ptr) :: ptr
>>ptr = c_loc (zzz(4:,4:))
>> where the compiler can easily tell that the argument is not contiguous ...
>> ?
>
> Definitely. I think there is also a PR about adding a
> gfc_is_simply_noncontiguous() or something like that. It has several uses:
> C_LOC, pointer-assignment to a contiguous pointer, removing some "if"s
> related to packing (as one knows that internal_pack will pack). And for
> compile-time simplification of the (unimplemented) Fortran 2008 function
> "is_contiguous".

Right: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45424

Cheers,
Janus


Re: [Patch, Fortran] PR56907 - do not 'pack' arrays passed to C_LOC

2013-04-20 Thread Tobias Burnus

Am 20.04.2013 12:42, schrieb Janus Weil:
Well, it does: As it doesn't know whether the array is contiguous or 
not - it packs the array. That's what one usually wants. However, for 
C_LOC one knows that the array is contiguous - the user promises this 
to the compiler - and, hence, no packing is needed. 

Ok, I see. Then the patch is certainly ok. (The fact that
conv_isocbinding_function is used also for C_ASSOCIATED and C_FUNLOC
is no problem either, I guess).

Well, the code in question is under:

  if (expr->value.function.isym->id == GFC_ISYM_C_LOC)



However, if calling C_LOC on a non-contiguous array is invalid,
shouldn't one add a check for cases like
   integer, dimension(1:5,1:5), target :: zzz
   type(c_ptr) :: ptr
   ptr = c_loc (zzz(4:,4:))
where the compiler can easily tell that the argument is not contiguous ... ?


Definitely. I think there is also a PR about adding a 
gfc_is_simply_noncontiguous() or something like that. It has several 
uses: C_LOC, pointer-assignment to a contiguous pointer, removing some 
"if"s related to packing (as one knows that internal_pack will pack). 
And for compile-time simplification of the (unimplemented) Fortran 2008 
function "is_contiguous".


Tobias


Re: [Patch, Fortran] PR56907 - do not 'pack' arrays passed to C_LOC

2013-04-20 Thread Janus Weil
>> What I don't quite understand is:
>> @@ -6317,8 +6317,13 @@ conv_isocbinding_function (gfc_se *se, gfc_expr
>> *expr)
>>   {
>> if (arg->expr->rank == 0)
>>   gfc_conv_expr_reference (se, arg->expr);
>> -  else
>> +  else if (gfc_is_simply_contiguous (arg->expr, false))
>>   gfc_conv_array_parameter (se, arg->expr, true, NULL, NULL, NULL);
>> +  else
>> +{
>> +  gfc_conv_expr_descriptor (se, arg->expr);
>> +  se->expr = gfc_conv_descriptor_data_get (se->expr);
>> +}
>>
>>
>> Why doesn't 'gfc_conv_array_parameter' handle this situation properly?
>
> Well, it does: As it doesn't know whether the array is contiguous or not -
> it packs the array. That's what one usually wants.
>
> However, for C_LOC one knows that the array is contiguous - the user
> promises this to the compiler - and, hence, no packing is needed.

Ok, I see. Then the patch is certainly ok. (The fact that
conv_isocbinding_function is used also for C_ASSOCIATED and C_FUNLOC
is no problem either, I guess).

However, if calling C_LOC on a non-contiguous array is invalid,
shouldn't one add a check for cases like

  integer, dimension(1:5,1:5), target :: zzz
  type(c_ptr) :: ptr
  ptr = c_loc (zzz(4:,4:))

where the compiler can easily tell that the argument is not contiguous ... ?

Cheers,
Janus