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

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

Hi,

Updated patch attached. Cleaned up issues found by contrib/check-GNU-style and
other misc. updates.

All changes to the whole patch set in comparison to the previous
submission are in the 005 diff.


On Mon, Aug 15, 2016 at 4:22 PM, Pekka Jääskeläinen <pekka@parmance.com> wrote:
> 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

Attachment: 005-diff-to-previous.patch.gz
Description: GNU Zip compressed data