On Fri, 2021-06-18 at 16:42 -0400, Antoni Boucher wrote: > I'm sending the patch once again for review. > > As it's the first time I'll land a patch, I'm not sure what needs to be > done when it's approved. > Do I just commit it to the master branch directly?
Commit (and push), yes, but... Please ensure the subject line of the commit matches our policy: "libgccjit: Handle truncation and extension for casts [PR95498]" is a good subject, whereas: "This patch handles truncation and extension for casts in jit." is not. We have some hooks that will only accept pushes if the commit message has a correctly formatted ChangeLog. The hooks ought to also add notification comments to bugzilla for any bugs mentioned in the commit message. See https://gcc.gnu.org/gitwrite.html#checkin FWIW in my own workflow I have a writable working copy that I keep for just doing pushes; I do a git pull verify the build, then git am FOO.patch --ignore-date to apply the patch that was tested and approved, then do a last test with that, then git push That way I only push the patches that I've tested and have been approved, and the other ones are in an entirely separate working copy. This may be overkill though. I'm also on #gcc IRC right now on OFTC (dmalcolm) if you run into issues. Dave > > Thanks! > > Le vendredi 11 juin 2021 à 13:49 -0400, David Malcolm a écrit : > > On Thu, 2021-05-27 at 21:22 -0400, Antoni Boucher wrote: > > > Here's the patch with the condition removed. > > > I believe everything is now fixed. > > > Thanks! > > > > Thanks; this looks good to me. Is this the latest version of the > > patch; would you like me to apply it? > > > > Dave > > > > > > > > Le jeudi 27 mai 2021 à 18:21 -0400, David Malcolm a écrit : > > > > On Tue, 2021-05-25 at 20:16 -0400, Antoni Boucher wrote: > > > > > I updated the patch according to the comments by Tom Tromey. > > > > > > > > > > There's one question left about your question regarding > > > > > C_MAYBE_CONST_EXPR, David: > > > > > > > > > > I am not sure if we can get a C_MAYBE_CONST_EXPR from > > > > > libgccjit, > > > > > and > > > > > it > > > > > indeed seems like it's only created in c-family. > > > > > However, we do use it in libgccjit here: > > > > > https://github.com/gcc-mirror/gcc/blob/master/gcc/jit/jit-playback.c#L1180 > > > > > > > > > > I tried removing the condition `if (TREE_CODE (t_ret) != > > > > > C_MAYBE_CONST_EXPR)` and all the tests of libgccjit still pass. > > > > > > > > > > That code was copied from here: > > > > > https://github.com/gcc-mirror/gcc/blob/master/gcc/c/c-convert.c#L175 > > > > > and might not be needed in libgccjit. > > > > > > > > > > Should I just remove the condition, then? > > > > > > > > I think so. > > > > > > > > Thanks > > > > Dave > > > > > > > > > > > > > > Le jeudi 13 mai 2021 à 19:58 -0400, David Malcolm a écrit : > > > > > > On Thu, 2021-05-13 at 19:31 -0400, Antoni Boucher wrote: > > > > > > > Thanks for your answer. > > > > > > > > > > > > > > See my answers below: > > > > > > > > > > > > > > Le jeudi 13 mai 2021 à 18:13 -0400, David Malcolm a écrit : > > > > > > > > On Sat, 2021-02-20 at 17:17 -0500, Antoni Boucher via > > > > > > > > Gcc- > > > > > > > > patches > > > > > > > > wrote: > > > > > > > > > Hi. > > > > > > > > > Thanks for your feedback! > > > > > > > > > > > > > > > > > > > > > > > > > Sorry about the delay in responding. > > > > > > > > > > > > > > > > In the past I was hesitant about adding more cast support > > > > > > > > to > > > > > > > > libgccjit > > > > > > > > since I felt that the user could always just create a > > > > > > > > union > > > > > > > > to > > > > > > > > do > > > > > > > > the > > > > > > > > cast. Then I tried actually using the libgccjit API to > > > > > > > > do > > > > > > > > this, > > > > > > > > and > > > > > > > > realized how much work it adds, so I now think we do want > > > > > > > > to > > > > > > > > support > > > > > > > > casting more types. > > > > > > > > > > > > > > > > > > > > > > > > > See answers below: > > > > > > > > > > > > > > > > > > On Sat, Feb 20, 2021 at 11:20:35AM -0700, Tom Tromey > > > > > > > > > wrote: > > > > > > > > > > > > > > > "Antoni" == Antoni Boucher via Gcc-patches > > > > > > > > > > > > > > > < > > > > > > > > > > > > > > > gcc-patches@gcc.gnu.org> writes: > > > > > > > > > > > > > > > > > > > > Antoni> gcc/jit/ > > > > > > > > > > Antoni> PR target/95498 > > > > > > > > > > Antoni> * jit-playback.c: Add support to > > > > > > > > > > handle > > > > > > > > > > truncation > > > > > > > > > > and extension > > > > > > > > > > Antoni> in the convert function. > > > > > > > > > > > > > > > > > > > > Antoni> + switch (dst_code) > > > > > > > > > > Antoni> + { > > > > > > > > > > Antoni> + case INTEGER_TYPE: > > > > > > > > > > Antoni> + case ENUMERAL_TYPE: > > > > > > > > > > Antoni> + t_ret = convert_to_integer (dst_type, > > > > > > > > > > expr); > > > > > > > > > > Antoni> + goto maybe_fold; > > > > > > > > > > Antoni> + > > > > > > > > > > Antoni> + default: > > > > > > > > > > Antoni> + gcc_assert > > > > > > > > > > (gcc::jit::active_playback_ctxt); > > > > > > > > > > Antoni> + gcc::jit::active_playback_ctxt- > > > > > > > > > > > add_error > > > > > > > > > > (NULL, > > > > > > > > > > "unhandled conversion"); > > > > > > > > > > Antoni> + fprintf (stderr, "input > > > > > > > > > > expression:\n"); > > > > > > > > > > Antoni> + debug_tree (expr); > > > > > > > > > > Antoni> + fprintf (stderr, "requested type:\n"); > > > > > > > > > > Antoni> + debug_tree (dst_type); > > > > > > > > > > Antoni> + return error_mark_node; > > > > > > > > > > Antoni> + > > > > > > > > > > Antoni> + maybe_fold: > > > > > > > > > > Antoni> + if (TREE_CODE (t_ret) != > > > > > > > > > > C_MAYBE_CONST_EXPR) > > > > > > > > > > > > > > > > Do we even get C_MAYBE_CONST_EXPR in libgccjit? That > > > > > > > > tree > > > > > > > > code > > > > > > > > is > > > > > > > > defined in c-family/c-common.def; how can nodes of that > > > > > > > > kind > > > > > > > > be > > > > > > > > created > > > > > > > > outside of the c-family? > > > > > > > > > > > > > > I am not sure, but that seems like it's only created in c- > > > > > > > family > > > > > > > indeed. > > > > > > > However, we do use it in libgccjit here: > > > > > > > > > > > > > > https://github.com/gcc-mirror/gcc/blob/master/gcc/jit/jit-playback.c#L1180 > > > > > > > > > > > > > > > > > > > > > > > > > Antoni> + t_ret = fold (t_ret); > > > > > > > > > > Antoni> + return t_ret; > > > > > > > > > > > > > > > > > > > > It seems weird to have a single 'goto' to maybe_fold, > > > > > > > > > > especially > > > > > > > > > > inside > > > > > > > > > > a switch like this. > > > > > > > > > > > > > > > > > > > > If you think the maybe_fold code won't be reused, > > > > > > > > > > then > > > > > > > > > > it > > > > > > > > > > should > > > > > > > > > > just > > > > > > > > > > be > > > > > > > > > > hoisted up and the 'goto' removed. > > > > > > > > > > > > > > > > > > This actually depends on how the support for cast > > > > > > > > > between > > > > > > > > > integers > > > > > > > > > and > > > > > > > > > pointers will be implemented (see below). > > > > > > > > > If we will support truncating pointers (does that even > > > > > > > > > make > > > > > > > > > sense? > > > > > > > > > and > > > > > > > > > I > > > > > > > > > guess we cannot extend a pointer unless we add the > > > > > > > > > support > > > > > > > > > for > > > > > > > > > uint128_t), that label will be reused for that case. > > > > > > > > > Otherwise, it might not be reused. > > > > > > > > > > > > > > > > > > So, please tell me which option to choose and I'll > > > > > > > > > update > > > > > > > > > my > > > > > > > > > patch. > > > > > > > > > > > > > > > > FWIW I don't think we'll want to support truncating or > > > > > > > > extending > > > > > > > > pointers. > > > > > > > > > > > > > > Ok, but do you think we'll want to support casts between > > > > > > > integers > > > > > > > and > > > > > > > pointers? > > > > > > > > > > > > Yes, though we probably want to reject truncating a pointer > > > > > > into > > > > > > a > > > > > > smaller integer type. > > > > > > > > > > > > > I opened an issue about this > > > > > > > (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95438) and > > > > > > > would > > > > > > > be > > > > > > > willing to do a patch for it eventually. > > > > > > > > > > > > > > > > > > > > > > > > > > On the other hand, if the maybe_fold code might be > > > > > > > > > > reused > > > > > > > > > > for > > > > > > > > > > some > > > > > > > > > > other > > > > > > > > > > case, then I suppose I would have the case end with > > > > > > > > > > 'break' > > > > > > > > > > and > > > > > > > > > > then > > > > > > > > > > have this code outside the switch. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > In another message, you wrote: > > > > > > > > > > > > > > > > > > > > Antoni> For your question, the current code already > > > > > > > > > > works > > > > > > > > > > with > > > > > > > > > > boolean and > > > > > > > > > > Antoni> reals and casts between integers and pointers > > > > > > > > > > is > > > > > > > > > > currently > > > > > > > > > > not > > > > > > > > > > Antoni> supported. > > > > > > > > > > > > > > > > > > > > I am curious why this wasn't supported. It seems > > > > > > > > > > like > > > > > > > > > > something > > > > > > > > > > that > > > > > > > > > > one might want to do. > > > > > > > > > > > > > > > > > > I have no idea as this is my first contribution to gcc. > > > > > > > > > But this would be indeed very useful and I opened an > > > > > > > > > issue > > > > > > > > > about > > > > > > > > > this: > > > > > > > > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95438 > > > > > > > > > > > > > > > > > > > thanks, > > > > > > > > > > Tom > > > > > > > > > > > > > > > > > > Thanks! > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
