Hi,
On Mon, May 16, 2016 at 08:25:57PM +0300, Pekka Jääskeläinen wrote:
> The BRIG frontend itself.
thanks making the effort to submit patches against trunk. I would be
very glad to see this included in the upcoming gcc version, not least
because it would make it easy to do basic testing of the HSA back-end
on any computer.
Even though I have spent considerable time reading the patches, I have
not yet read it all, but I think I have accumulated enough notes to
post them back. Also please note that I have actually never written a
front-end myself so it can easily happen that on many accounts you
will be right and my feedback wrong. I hope it will still be useful,
though.
Generally speaking, I think you have made quite a good effort trying
to adhere to the GNU coding standard (you know, blanks before
parenthesis and stuff), but:
- 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).
- We very much prefer c-style comments to c++ ones. I hope they can
be converted easily in some automated way.
So far I have the following specific comments:
- brig-c.h
+ please remove commented out extern GTY (()) tree brig_non_zero_struct
- 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.
+ brig_langhook_handle_option has argument scode marked as unused
but it is used.
+ brig_langhook_type_for_size uses both supposedly unused arguments
and I am surprised that handling just 64 bits is sufficient.
+ 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
+ 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.
- 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?
- 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.
+ Is there any reason why you use internal_error instead of
more common gcc_assert and/or gcc_unreachable?
- brigfrontend/brig_to_generic.cc:
+ Why does this filename have underscores while all the others have
dashes? :-)
+ What should the sanity check of data secion in parse accomplish?
+ 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?
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.
+ 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.
+ Remove the commented out debug_function in
brig_to_generic::finish_function.
+ 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?
- 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.
+ 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.
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.
+ 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.
+ 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.
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.
How come that the "segment == BRIG_SEGMENT_ARG" is the only case
in which you have to handle arrays?
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.
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.
When you add pointer and offsets, you should be using
POINTER_PLUS_EXPR rather than just PLUS_EXPR.
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.
+ 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?
+ brig_code_entry_handler::add_temp_var: Seems like he coment is
wrong, the assignment is appended to the current function but not
returned.
+ 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.
+ 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?
+ brig_code_entry_handler::build_operands: Please use
BRIG_TYPE_BASE_MASK instead of the 0x01f constant when masking
types.
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.
+ 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.
- 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.
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.
- 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).
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.
I'll continue reading the rest,
Martin
