Hi Martin, On Fri, Nov 4, 2016 at 4:58 PM, Martin Jambor <mjam...@suse.cz> wrote: > I personally primarily want this for debugging purposes, and we should > try to eventually report errors in a more comprehensible way than > HSAILasm. But more generally, and more importantly, if the input, > whether human readable or not, is incorrect, the compiler should not > produce an "internal error." If that happens, users will file bugs > against gcc when in fact it is the generating tool that is broken. > One you maintain the FE, you personally will not want this :-)
Yes, I will add better error messages as soon asissues are reported. > I was wondering why in case of VECTOR_TYPE, instead of > return build1 (VIEW_CONVERT_EXPR, type, expr); > you do not do > return fold (convert_to_vector (type, expr)); > > it seemed to me natural given how you handle the other cases and that > the function internally also builds a V_C_E, after two checks. So I > thought the checks (matching overall size and only converting from an > integer or another vector-type) were the problem and was wondering > which one. Especially the size one is something I believe you should > never violate too. I see. I cannot recall the origins of this, but my guess is that the code is copied from an older Go frontend version. I tested with your suggestion and it seems to work fine. > copy_move_inst_handler operator() starts by type-casting whatever it > gets to BrigInstSourceType* and immediately dereferencing it, loading > field sourceType, even though, in case of an LDA, what it is actually > looking at is not a BrigInstSourceType but a BrigInstAddr, which is an > entirely different structure that does not have that field. So you > read the 8-bit segment and the first reserved field and happily pass > that to gccbrig_tree_type_for_hsa_type, hoping it does not explode on > random data. That is wrong. > > Later on in the operator() you actually figure out that you are > looking at BrigInstSourceType but that is too late. Ah, I see the issue now. It worked as with LDA the input type was forced and handled explicitly. I now made the code more explicit regarding this. > Indeed it does, thanks. However, I think that at the point when you > do: > > if (offs > 0) > const_offset = build_int_cst (size_type_node, offs); > > const_offset can already be non-NULL you may lose whatever value it > had before. You might want to keep the constant offset as integer > until the very moment when you may want to use it. Great catch! There indeed was a major memory corruption with group and private variable which was not caught by the PRM conformance suite I use for testing. Should be now fixed. > (Also, single statements should not have braces around them ;-) ...also fixed these, once again ;) > I see. If you think that MULT_HIGHPART_EXPR is broken, it may be > worth filing a bug report (probably during next stage1). It can be considered broken or not just fully implemented for all targets and various vector types (especially those not directly supported by the target). Yes, I rather look closer and report this after the patch has been merged to be able to provide BRIG test cases that work with upstream code base. > This week I have checked out the updated tree from github and looked > at only a few changed functions there. Hopefully the steering > committee will decide soon and we'll get attention of a global > reviewer during the next few weeks too. > > Thanks for addressing so many of my comments, Thanks again, an updated FE patch attached. BR, Pekka
002-brig-fe-gcc.patch.gz
Description: GNU Zip compressed data
