On 12/14/21 8:23 PM, Segher Boessenkool wrote: > On Tue, Dec 14, 2021 at 07:32:30AM -0600, Bill Schmidt wrote: >> On 12/13/21 6:22 PM, Segher Boessenkool wrote: >>> On Mon, Dec 13, 2021 at 02:37:43PM -0600, Bill Schmidt wrote: >>>> On 12/13/21 10:54 AM, Segher Boessenkool wrote: >>>>> On Mon, Dec 13, 2021 at 11:30:28AM -0500, David Edelsohn wrote: >>>>>> On Mon, Dec 13, 2021 at 10:48 AM Bill Schmidt <wschm...@linux.ibm.com> >>>>>> wrote: >>>>>>> PR103624 observes that we get segfaults for the 64-bit darn builtins >>>>>>> when compiled >>>>>>> on a 32-bit architecture. The old built-in infrastructure requires >>>>>>> TARGET_64BIT, and >>>>>>> this was missed in the new support. Moving these two builtins from the >>>>>>> [power9] >>>>>>> stanza to the [power9-64] stanza solves the problem. >>>>>>> >>>>>>> Tested the fix on a powerpc-e300c3-linux-gnu cross. Bootstrapped and >>>>>>> tested on >>>>>>> powerpc64le-linux-gnu with no regressions. Is this okay for trunk? >>>>>> Okay. >>>>> No, as I said before this is not correct, not without a lot more >>>>> explanation at least. We should not copy errors in the old code into >>>>> the new code. That is negating one of the main advantages of >>>>> reimplementing this in the first place! >>>> Can you please be more specific? >>>> >>>> All I have from you before is "It should work for 32-bit though?" I >>>> responded in the >>>> bug report that __builtin_darn_32 was used for this purpose. I haven't >>>> seen a >>>> response to that. What do you want to see happen? >>> That of course does not work for _raw. >>> >>> These builtins should just return a "long", just like __builtin_ppc_mftb >>> does. All three of them. >> Well, that seems wrong for __builtin_darn_32, which maps to an SImode >> pattern. > That is Yet Another Bug, then. > > The insn returns a full register. The patterns should use either :P or > :GPR (the latter if SImode makes sense for it, so we could have that for > all darn variants). :DI and :SI never make sense for this. > >> So, I assume what you'd like to see is for the other two built-ins to return >> long, and for the "&& TARGET_64BIT" to be removed from the darn_raw and darn >> patterns? > No, all builtins should work in either mode, and always return long. > If the patterns are broken, the *patterns* should be fixed :-)
OK, thanks! This is much clearer now. I've opened an internal issue about the deficiencies of the darn patterns and their associated built-ins. In response to PR103624, I would like to start with the existing patch to ensure the new support mirrors what we had before, so we have that as a baseline. We can then move on to fixing the larger set of problems. Is that a reasonable plan? Thanks! Bill > >>> Avoiding ICEs should not be a goal. It should be a side effect of doing >>> the right thing in the first place! >> There's no reason to get snippy. Given that you approved Kelvin's original >> implementation of the darn patterns and built-in functions, I think I can be >> forgiven for thinking that those were the desired semantics. :-) > Sorry if I sound annoyed. I am annoyed, but not with you. Just with > the world in general I suppose. > > With the new builtins representation it is much easier to spot problems, > it is a great success already! > > > Segher
