Re: [PATCH 2/4] BRIG (HSAIL) frontend: The FE itself.

[Pekka Jääskeläinen <pekka at parmance dot com>]

Thanks a lot for the review Martin,

Responses inline:

On Mon, Aug 1, 2016 at 7:37 PM, Martin Jambor <mjambor@suse.cz> wrote:
>
>   - I would definitely appreciate more comments.  All but the most
>     trivial functions should have one that describes what the function
>     does, what are its arguments and what it returns.  (And there
>     should be one blank line between the comment and the function
>     itself).


I added more comments. Please let me know if I missed a non-trivial one.

>
>
>   - We very much prefer c-style comments to c++ ones.  I hope they can
>     be converted easily in some automated way.


Converted.

>
>
> So far I have the following specific comments:
>
> - brig-c.h
>   + please remove commented out extern GTY (()) tree brig_non_zero_struct


Done.

>
>
> - brig-lang.c:
>   + In the long run I would like to get rid of
>       opts->x_flag_whole_program = 0 in
>     brig_langhook_init_options_struct, when did it cause issues, when
>     you tried LTO?  Since there obviously is no linker-plugin support,
>     I think we can leave it this way for now, though.


Agreed. I have not tried the current status of LTO, but it can be implemented
later.


>
>   + brig_langhook_handle_option has argument scode marked as unused
>     but it is used.


Fixed.

>
>   + brig_langhook_type_for_size uses both supposedly unused arguments
>     and I am surprised that handling just 64 bits is sufficient.


It is now being called for 32b too due to introducing the builtins like
they should.

>
>   + brig_langhook_type_for_mode: the "FIXME: This static_cast should
>     be in machmode.h" can be removed, the cast is in machmode.h
>
>   + brig_langhook_eh_personality comment refers to file
>     libbrig/runtime/brig-unwind.c which does not seem to exist?
>   + convert has attributes marked as unused but they are used


Fixed these.

>
>   + The "FIXME: This is a hack to preserve trees that we create from
>     the garbage collector." IMHO does not describe any real issue,
>     using GTY roots for that is common practice.


Removed the FIXME.

> - brigspec.c:
>   + lang_specific_driver: if (len > 3 && strcmp (arg + len - 3,
>     ".brig") == 0) can never be true, even if str end with brig.
>     Consequently, the if branch marked by FIXME later on in the
>     function never gets executed.  So perhaps it is not necessary?


True. A copy-paste error.
>
>
> - brigfrontend/*.cc in general:
>
>   + A lot of functions should have a comment.  I prefer to have them
>     above the function body but if it is a method, a comment in the
>     class definition is fin e too (I think).  Often you have helpful
>     comments inside the function but it really helps if you know what
>     to expect before you start reading the function.  For example,
>     brig_to_generic::add_global_variable needs a comment that it adds
>     VAR_DECL to the list of global variables and if there is a "host
>     def var" (I guess I know what that means but an explanation
>     somewhere would not hurt either), it makes sure it points to the
>     new VAR_DECL.  Another example: call_builtin is very difficult to
>     review without a comment explaining what arguments it expects.
>     Please make sure that all functions have a comment somewhere,
>     perhaps with the exception of only the most trivial and
>     self-evident.


Fixed these. I also scanned the sources and added new comments
where e.g. the function name didn't describe the purpose in a self clear
way.


>
>   + Is there any reason why you use internal_error instead of
>     more common gcc_assert and/or gcc_unreachable?


I think it was because of the possibility to add a format str & a message
more easily while developing. I now converted them to asserts/unreachable
as the frontend can be considered feature complete.


>
> - brigfrontend/brig_to_generic.cc:
>
>   + Why does this filename have underscores while all the others have
>     dashes? :-)


Renamed.

>
>   + What should the sanity check of data secion in parse accomplish?


Nothing anymore. A leftover from debugging sessions, I believe.
Removed.

>   + In build_reinterpret_cast, it would be best if you could avoid
>     constructing VIEW_CONVERT_EXPRs that have type with a different
>     size from the type of its operand (even though I think that Ada
>     also does this, it is considered unfortunate).  What are the cases
>     when this happens?


This was needed due to the special treatment of f16 which is stored in
a 32b reg (variable). I added a comment about this.

>
>     OK, adding another note later: For converting scalars (anything
>     !AGGREGATE_TYPE_P), I think you pretty much always want to use
>     NOP_EXPR rather than generating a V_C_E.  V_C_E is mainly used to
>     change how we interpret aggregates (or type-cast between
>     aggregates and scalars).  In particular, NOP_EXPR will also
>     sign-extend properly when you are extending intergers according to
>     the type, whereas what will be in the "extended" part of a V_C_E
>     is basically undefined.


I use VCE heavily on purpose in many places because of the untyped HSAIL
register variables (represented as unsigned integer local variables) of which
contents are treated as different types depending on the HSAIL opcode.
The integer (extending) casts are done with separate HSAIL instructions.
I don't want to sign extend unless explicitly stated so.

>
>   + in brig_to_generic::append_group_variable, I think you should not
>     add align_padding to m_next_group_offset twice.  You do not do
>     that in append_private_variable.


Good catch. Fixed.

>
>
>   + Remove the commented out debug_function in
>     brig_to_generic::finish_function.


I put it inside #if 0 ... #endif instead as it's a very useful snippet
to have handy when
debugging the output.

>   + call_builtin - difficult to review without a comment explaining
>     the parameters.  Apart from that, I really wonder why you go
>     through the hoops of identifying builtins by names and caching
>     them.  I believe you should be able to use builtin_decl_explicit
>     to get the decl for the builtin code. Or am I wrong?


Converted to use the standard way of importing builtins.def.
I mimicked lto-lang.c.

>
> - brigfrontend/brig-code-entry-handler.cc:
>
>   + I don't know, maybe it is standard C++ practice, but putting
>     whole-front-end initialization (built-in creation) to a
>     constructor of an abstract ancestor of streaming class seems a bit
>     unfortunate to me.


This is now reduced to initialization of the builtin index map from the
hsail-builtin.defs and the actual builtins are initialized in brig-lang.c.

>
>   + Speaking about built-ins.  I know that GO FE creates them like you
>     do, but I wonder whether following how c-family/c-common.c creates
>     them would not be better, at least for those builtins that are in
>     builtins.def and files included from it.  That way you also get
>     proper function attributes without copying them over, for example.
>     However, is is entirely possible there is a reason to do it
>     your/Go way, I was just surprised.


Moved all builtins HSAIL use to hsail-builtins.def.

>     You should probably also go forward with the TODO and make the
>     add_custom_builtin calls into normal gcc builtins, otherwise LTO
>     will not work with BRIG.


Done.

>   + brig_code_entry_handler::build_tree_operand: Please make it just
>     one switch, rather than two if's followed by a switch, all of them
>     based on the same field.


Done.

>
>   + brig_code_entry_handler::build_address_operand: I think that
>
>         gcc_assert (arg_symbol->base.kind == BRIG_KIND_DIRECTIVE_VARIABLE);
>
>     checks for input error, rather than internal compiler error, and
>     so if the symbol is not a symbol, you should call error() and bail
>     out instead.  I think that given the nature of brig - there is
>     little point in trying to continue parsing after an error -
>     terminating the executable would be fine too.


Makes sense. Removed the assert.

>
>     Also, in the calculation of the symbol_base for private segment
>     symbols, I bet the third call to expand_or_call_builtin:
>
>           local_size
>             = build2 (MULT_EXPR, uint32_type_node,
>                       expand_or_call_builtin (BRIG_OPCODE_WORKGROUPSIZE,
>                                               BRIG_TYPE_U32, uint32_type_node,
>                                               uint32_1),
>                       local_size);
>
>     should have uint32_2 as its last argument to get the 3rd dimension
>     of the grid.


Yes, this was already fixed recently.

>     How come that the "segment == BRIG_SEGMENT_ARG" is the only case
>     in which you have to handle arrays?


Added this comment there to clarify it:

       /* Two different type of array references in case of arguments
depending where they are referred at.  In the caller (argument
segment), the reference is to an array object and
in the callee, the array object has been passed as a pointer
to the array object.  */

For the other segments, we handle array refs via addressing the first
element in which case it doesn't differ from "scalar" variables.

>     It also seems that if the address is from a KERNARG segment but
>     also has a register component, you will overwrite the kernel
>     base stored to ptr_base when dealing with the register.


 Good catch. Fixed and added a test case.
>
>
>     The code processing the register value should be done differently,
>     I'm afraid.  First and foremost, you should not be using
>     VIEW_CONVERT_EXPR (generated by build_reinterpret_cast) to
>     sign-extend offsets, you should use NOP_EXPR for that.  I also do
>
>     not think there is any need to be constructing build_pointer_type
>     (char_type_node) all the time.  GIMPLE/GENERIC is not C, you can
>     use any pointer, for example ptr_type_node.


Thanks for the simplification tips. I converted this part to pretty much a call
to convert_to_pointer().

>
>     When you add pointer and offsets, you should be using
>     POINTER_PLUS_EXPR rather than just PLUS_EXPR.


Fixed. For some reason I thought POINTER_PLUS_EXPR requires a constant
integer offset.

>
>     Finally, if you want to type-cast the final address to the type
>     specified by the instruction, again use NOP_EXPR.  However, I
>     don't think it is really necessary, in the middle-end, it is the
>     type of the loads and stores that matter, not really the used
>     pointer.


Not sure of this. Isn't the mem access' size&alignment dictated by the
pointer's type? At least that's the impression I get from MEM_REF's
comment.

>
>   + brig_code_entry_handler::get_tree_expr_type_for_hsa_type: Needs a
>     comment.  Why do we need it, in what way is it different than the
>     previous function?  It special-cases F16 vectors, but differently,
>     so which function should be used in which situation?


Improved the comment.

>
>   + brig_code_entry_handler::add_temp_var: Seems like he coment is
>     wrong, the assignment is appended to the current function but not
>     returned.


Fixed. It should have returned it, and gcc's default behavior of
returning the last expression came into play, I think.

>   + brig_code_entry_handler::unpack: Please use BITS_PER_UNIT to refer
>     to the number of bits in a byte instead of using the number 8
>     directly.


Fixed.

>   + brig_code_entry_handler::vbuild_builtin: A comment would help
>     greatly.  Moreover, it seems weird that you are building an
>     arbitrary(?)  type for a builtin, why don't you get it from the
>     decl?


This method is not needed anymore, thanks to the builtin rework.

>
>   + brig_code_entry_handler::build_operands: Please use
>     BRIG_TYPE_BASE_MASK instead of the 0x01f constant when masking
>     types.


Fixed, also elsewhere.

>
>     The need to always encapsulate constant(?) operands in a V_C_E is
>     very strange and I really think we need to figure out what is
>     going on.


I implemented the FE originally on an earlier gcc version and added
some workarounds
such as this, hoping that the issues were fixed in the later versions.

Seems this workaround is not needed anymore with gcc trunk, removed...

>
>
>   + brig_code_entry_handler::build_output_assignment: Again, Please
>     use BRIG_TYPE_BASE_MASK instead of the 0x01f and BITS_PER_UNIT
>     instead of 8.  The most worisome, however, is:
>
>
>           if (CONVERT_EXPR_P (inst_expr) && POINTER_TYPE_P (inst_expr))
>             {
>               // convert_to_integer crashes when converting a view convert
>               // expr to a pointer.  First cast it to a large enough int
>               // and let the next integer conversion do the clipping.
>               inst_expr = convert_to_integer (size_type_node, inst_expr);
>             }
>
>     First of all, the above should not help, because V_C_E is not a
>     CONVERT_EXPR_P.  Mainly, however, this should either be reported
>     as a bug and fixed or probably avoided by not creating so many
>     V_C_Es in the first place.


...ditto.

> - brigfrontend/brig-atomic-inst-handler.cc: generate_tree - I have not
>   read this file in much detail mainly because I'm still not convinced
>   that organizing builtins based on their name strings is a good idea.


Reworked.

>   However, I'd like alto to point out that you should try to avoid
>   building trees if you are not going to use them, especially if it is
>   really simple like for example mem_ref in this function, because it
>   strains the garbage collector for no reason.


Fixed.

>
> - brig-basic-inst-handler.cc, operator():
>
>   if (brig_inst->opcode == BRIG_OPCODE_NOP)
>     {
>       return base->byteCount;
>     }
>
>   tere should be no braces when there is only one statement in a
>   branch (and there is no potential confusion about elses).


Fixed.

>
>   Again, please do not use 0x01F to mask brug type base, use
>   BRIG_TYPE_BASE_MASK.  Uou can also use BRIG_TYPE_PACK_MASK instead
>   of or'ing all the pack values.


Fixed.

Attached an updated patch. Also the other patches got touched a bit, I attach
them to the respective threads.

Thanks again,
Pekka

Attachment: 002-brig-fe-gcc.patch.gz
Description: GNU Zip compressed data