| Message ID | alpine.LSU.2.11.1602150952320.1392@t29.fhfr.qr |
|---|---|
| State | New |
| Headers | show |
> So you simply assume that exp_type is naturally aligned here. I think > you should test align < TYPE_ALIGN (TYPE_MAIN_VARIANT (exp_type)) here, > no? No strong opinion, but the patch is only intended to mitigate the effects of the PR65310 one-liner, including on the 5 branch, so it's minimal. > And if you get enough supporters to apply this kind of workaround > I'd prefer it to be in build_aligned_type itself, basically > refusing to build over-aligned types. I pondered that but rejected it for the 5 branch. > And I'd rather make this controlled by an internal flag that backends should > consciously set (aka a target hook). I disagree, the onus is on the middle-end to fix its mess and the default should not be to generate wrong code in any case. > The above is simply a bit too ugly IMHO > and looks incomplete(?), cannot even the cummulative args machinery > end up with type-align specifics or are you sure those can only > be triggered from function_arg_boundary? I think that testing function_arg_boundary is a conservative criterion, but I admittedly didn't check every architecture. > Note the real issue is overaligned register types. Agreed, and IMO the middle-end should not create them out of nowhere. > At least it seems you have a testcase that reproduces the error > so you should be able to point to a specific pass doing the wreckage > and provide preprocessed source and a cross-compiler setup to > investigate. I'm just using Jeff's reduced testcase for MIPS.
On Fri, 19 Feb 2016, Eric Botcazou wrote: > > So you simply assume that exp_type is naturally aligned here. I think > > you should test align < TYPE_ALIGN (TYPE_MAIN_VARIANT (exp_type)) here, > > no? > > No strong opinion, but the patch is only intended to mitigate the effects of > the PR65310 one-liner, including on the 5 branch, so it's minimal. > > > And if you get enough supporters to apply this kind of workaround > > I'd prefer it to be in build_aligned_type itself, basically > > refusing to build over-aligned types. > > I pondered that but rejected it for the 5 branch. > > > And I'd rather make this controlled by an internal flag that backends should > > consciously set (aka a target hook). > > I disagree, the onus is on the middle-end to fix its mess and the default > should not be to generate wrong code in any case. > > > The above is simply a bit too ugly IMHO > > and looks incomplete(?), cannot even the cummulative args machinery > > end up with type-align specifics or are you sure those can only > > be triggered from function_arg_boundary? > > I think that testing function_arg_boundary is a conservative criterion, but I > admittedly didn't check every architecture. > > > Note the real issue is overaligned register types. > > Agreed, and IMO the middle-end should not create them out of nowhere. Yup, so we should make sure we don't (even not "out of nowhere"). See my attempts on adding some SSA verification for this (needs to be restricted to overaligned, then it doesn't trigger that often...). One issue is that we've often got DECLs that are overaligned but at some point re-write them into SSA - and the SSA verifier expects the SSA name types to agree with their decls type. I didn't yet try to fixup DECL alignment to be natural as we re-write it into SSA but I believe that may be necessary in the end to make such SSA verification not trigger. > > At least it seems you have a testcase that reproduces the error > > so you should be able to point to a specific pass doing the wreckage > > and provide preprocessed source and a cross-compiler setup to > > investigate. > > I'm just using Jeff's reduced testcase for MIPS. See the bug where I fixed up some more places in SRA after debugging a cross to MIPS with the reduced testcase (fixing verification fails with added SSA alignment checking). Richard.
On 02/19/2016 09:36 AM, Richard Biener wrote: > Yup, so we should make sure we don't (even not "out of nowhere"). See > my attempts on adding some SSA verification for this (needs to be > restricted to overaligned, then it doesn't trigger that often...). > One issue is that we've often got DECLs that are overaligned but at > some point re-write them into SSA - and the SSA verifier expects the > SSA name types to agree with their decls type. Sounds like the verifier needs to be relaxed a little? Bernd
On Fri, 19 Feb 2016, Bernd Schmidt wrote: > On 02/19/2016 09:36 AM, Richard Biener wrote: > > Yup, so we should make sure we don't (even not "out of nowhere"). See > > my attempts on adding some SSA verification for this (needs to be > > restricted to overaligned, then it doesn't trigger that often...). > > One issue is that we've often got DECLs that are overaligned but at > > some point re-write them into SSA - and the SSA verifier expects the > > SSA name types to agree with their decls type. > > Sounds like the verifier needs to be relaxed a little? No, we really use those two types interchangeably. If it's a DECL_INGORED var adjusting it is fine I think, but if it's a user var we'd drop some debug info (do we actually emit alignment info?) and if it's a PARM_DECL then it may change the ABI (in which case their SSA names maybe should have overaligned type...) Richard. > > Bernd > >
On Fri, Feb 19, 2016 at 01:42:20PM +0100, Richard Biener wrote: > On Fri, 19 Feb 2016, Bernd Schmidt wrote: > > > On 02/19/2016 09:36 AM, Richard Biener wrote: > > > Yup, so we should make sure we don't (even not "out of nowhere"). See > > > my attempts on adding some SSA verification for this (needs to be > > > restricted to overaligned, then it doesn't trigger that often...). > > > One issue is that we've often got DECLs that are overaligned but at > > > some point re-write them into SSA - and the SSA verifier expects the > > > SSA name types to agree with their decls type. > > > > Sounds like the verifier needs to be relaxed a little? > > No, we really use those two types interchangeably. If it's > a DECL_INGORED var adjusting it is fine I think, but if it's a > user var we'd drop some debug info (do we actually emit alignment I think we don't right now, but DWARF5 is going to have attributes for this, so we'll start soon. > info?) and if it's a PARM_DECL then it may change the ABI > (in which case their SSA names maybe should have overaligned type...) Jakub
Index: tree-ssa.c =================================================================== --- tree-ssa.c (revision 233369) +++ tree-ssa.c (working copy) @@ -936,6 +936,22 @@ verify_ssa (bool check_modified_stmt, bo name, stmt, virtual_operand_p (name))) goto err; } + + if (! SSA_NAME_VAR (name) + && TYPE_MAIN_VARIANT (TREE_TYPE (name)) != TREE_TYPE (name)) + { + error ("type is not main variant"); + goto err; + } + but that's not too happy (to verify the change below). But verifying we don't have over-aligned (or under-aligned) registers would be a good thing and I think all registers should have main-variant types. So I'd rather nail down the case that still gets through with the below patch using sth like the above (well, as said, adjust it to check all SSA names and TYPE_ALING == TYPE_ALIGN (TYPE_MAIN_VARIANT ())). At least it seems you have a testcase that reproduces the error