On Tue, 2024-10-29 at 07:59 -0400, Antoni Boucher wrote: > David: Arthur reviewed the gccrs patch and would be OK with it. > > Could you please take a look and review it?
https://github.com/Rust-GCC/gccrs/pull/3195 looks good to me; thanks! Dave > > Le 2024-10-17 à 11 h 38, Antoni Boucher a écrit : > > Hi. > > Thanks for the review, David! > > > > I talked to Arthur and he's OK with having a file to include in > > both > > gccrs and libgccjit. > > > > I sent the patch to gccrs to move the code in a new file that we > > can > > include in both frontends: > > https://github.com/Rust-GCC/gccrs/pull/3195 > > > > I also renamed gcc_jit_target_info_supports_128bit_int to > > gcc_jit_target_info_supports_target_dependent_type because a > > subsequent > > patch will allow to check if other types are supported like > > _Float16 and > > _Float128. > > > > Here's the patch for libgccjit updated to include this file. > > > > Thanks. > > > > Le 2024-06-26 à 17 h 55, David Malcolm a écrit : > > > On Sun, 2024-03-10 at 12:05 +0100, Iain Buclaw wrote: > > > > Excerpts from David Malcolm's message of März 5, 2024 4:09 pm: > > > > > On Thu, 2023-11-09 at 19:33 -0500, Antoni Boucher wrote: > > > > > > Hi. > > > > > > See answers below. > > > > > > > > > > > > On Thu, 2023-11-09 at 18:04 -0500, David Malcolm wrote: > > > > > > > On Thu, 2023-11-09 at 17:27 -0500, Antoni Boucher wrote: > > > > > > > > Hi. > > > > > > > > This patch adds support for getting the CPU features in > > > > > > > > libgccjit > > > > > > > > (bug > > > > > > > > 112466) > > > > > > > > > > > > > > > > There's a TODO in the test: > > > > > > > > I'm not sure how to test that gcc_jit_target_info_arch > > > > > > > > returns > > > > > > > > the > > > > > > > > correct value since it is dependant on the CPU. > > > > > > > > Any idea on how to improve this? > > > > > > > > > > > > > > > > Also, I created a CStringHash to be able to have a > > > > > > > > std::unordered_set<const char *>. Is there any built-in > > > > > > > > way > > > > > > > > of > > > > > > > > doing > > > > > > > > this? > > > > > > > > > > > > > > Thanks for the patch. > > > > > > > > > > > > > > Some high-level questions: > > > > > > > > > > > > > > Is this specifically about detecting capabilities of the > > > > > > > host > > > > > > > that > > > > > > > libgccjit is currently running on? or how the target was > > > > > > > configured > > > > > > > when libgccjit was built? > > > > > > > > > > > > I'm less sure about this part. I'll need to do more tests. > > > > > > > > > > > > > > > > > > > > One of the benefits of libgccjit is that, in theory, we > > > > > > > support > > > > > > > all > > > > > > > of > > > > > > > the targets that GCC already supports. Does this patch > > > > > > > change > > > > > > > that, > > > > > > > or > > > > > > > is this more about giving client code the ability to > > > > > > > determine > > > > > > > capabilities of the specific host being compiled for? > > > > > > > > > > > > This should not change that. If it does, this is a bug. > > > > > > > > > > > > > > > > > > > > I'm nervous about having per-target jit code. Presumably > > > > > > > there's a > > > > > > > reason that we can't reuse existing target logic here - > > > > > > > can you > > > > > > > please > > > > > > > describe what the problem is. I see that the ChangeLog > > > > > > > has: > > > > > > > > > > > > > > > * config/i386/i386-jit.cc: New file. > > > > > > > > > > > > > > where i386-jit.cc has almost 200 lines of nontrivial > > > > > > > code. > > > > > > > Where > > > > > > > did > > > > > > > this come from? Did you base it on existing code in our > > > > > > > source > > > > > > > tree, > > > > > > > making modifications to fit the new internal API, or did > > > > > > > you > > > > > > > write > > > > > > > it > > > > > > > from scratch? In either case, how onerous would this be > > > > > > > for > > > > > > > other > > > > > > > targets? > > > > > > > > > > > > This was mostly copied from the same code done for the Rust > > > > > > and D > > > > > > frontends. > > > > > > See this commit and the following: > > > > > > https://gcc.gnu.org/git/? > > > > > > p=gcc.git;a=commit;h=b1c06fd9723453dd2b2ec306684cb806dc2b4f > > > > > > bb > > > > > > The equivalent to i386-jit.cc is there: > > > > > > https://gcc.gnu.org/git/? > > > > > > p=gcc.git;a=commit;h=22e3557e2d52f129f2bbfdc98688b945dba28d > > > > > > c9 > > > > > > > > > > [CCing Iain and Arthur re those patches; for reference, the > > > > > patch > > > > > being > > > > > discussed is attached to : > > > > > https://gcc.gnu.org/pipermail/jit/2024q1/001792.html ] > > > > > > > > > > One of my concerns about this patch is that we seem to be > > > > > gaining > > > > > code > > > > > that's per-(frontend x config) which seems to be copied and > > > > > pasted > > > > > with > > > > > a search and replace, which could lead to an M*N explosion. > > > > > > > > > > > > > That's certainly the case with the configure/make rules. Itself > > > > I > > > > think > > > > is copied originally from the {cpu_type}-protos.h machinery. > > > > > > > > It might be worth pointing out that the c-family of front-ends > > > > don't > > > > have separate headers because their per-target macros are > > > > defined in > > > > {cpu_type}.h directly - for better or worse. > > > > > > > > > Is there any real difference between the per-config code for > > > > > the > > > > > different frontends, or should there be a general "enumerate > > > > > all > > > > > features of the target" hook that's independent of the > > > > > frontend? > > > > > (but > > > > > perhaps calls into it). > > > > > > > > > > > > > As far as I understand, the configure parts should all be > > > > identical > > > > between tm_p, tm_d, tm_rust, ..., so would benefit from being > > > > templated > > > > to aid any other front-ends adding in their own per target > > > > hooks. > > > > > > > > > Am I right in thinking that (rustc with default LLVM backend) > > > > > has > > > > > some > > > > > set of feature strings that both (rustc with > > > > > rustc_codegen_gcc) and > > > > > gccrs are trying to emulate? If so, is it presumably a goal > > > > > that > > > > > libgccjit gives identical results to gccrs? If so, would it > > > > > be > > > > > crazy > > > > > for libgccjit to consume e.g. config/i386/i386-rust.cc ? > > > > > > > > I don't know whether libgccjit can just pull in directly the > > > > implementation of the rust target hooks here. > > > > > > Sorry for the delay in responding. > > > > > > I don't want to be in the business of maintaining a copy of the > > > per- > > > target code for "jit", and I think it makes sense for libgccjit > > > to > > > return identical information compared to gccrs. > > > > > > So I think it would be ideal for jit to share code with rust for > > > this, > > > rather than do a one-time copy-and-paste followed by a ongoing > > > "keep > > > things updated" treadmill. > > > > > > Presumably there would be Makefile.in issues given that e.g. > > > Makefile > > > has i386-rust.o listed in: > > > > > > # Target specific, Rust specific object file > > > RUST_TARGET_OBJS= i386-rust.o linux-rust.o > > > > > > One approach might be to move e.g. the i386-rust.cc code into, > > > say, a > > > i386-rust-and-jit.inc file and #include it from i386-rust.cc and > > > i386- > > > jit.cc (with a similar approach for other targets) > > > > > > Antoni, Arthur, would you each be OK with that? > > > > > > > > > > The per-frontend target > > > > hooks usually also make use of code specific to that front-end > > > > - > > > > TARGET_CPU_CPP_BUILTINS and others can't be used by a non-c- > > > > family > > > > front-end without adding a plethora of stubs, for example. > > > > > > > > Whether or not libgccjit wants to give identical information as > > > > as > > > > rust > > > > I think is a decision for you as the maintainer of its API. > > > > > > Also a question for Antoni and Arthur: is that desirable for the > > > two > > > gcc rust approaches? > > > > > > Thanks > > > Dave >
