On June 4, 2020 6:20:36 PM GMT+02:00, Eric Botcazou <botca...@adacore.com> wrote: >> I'll go with this variant since it is more obvious unless I hear >> otherwise. > >Yes, at least this one doesn't appear to further confuse an already >confusing >implementation. ;-) > >> Thanks, >> Richard. >> >> >> The following patch avoids keeping the inherited MEM_ATTRS when >> set_mem_attributes_minus_bitpos is called with a variable ARRAY_REF. >> The inherited ones may not reflect the correct offset and neither >> does the updated alias-set match the inherited MEM_EXPR. This all >> ends up confusing path-based alias-analysis, causing wrong-code. > >"variable ARRAY_REF" is just an ARRAY_REF with variable index or an >ARRAY_REF >whose base is variable? In other words, do we end up taking the > /* Else do not record a MEM_EXPR. */ > >path instead of setting attrs.expr again?
Yes, we're taking this path in the problematical case. >> The fix is to stop not adopting a MEM_EXPR for certain kinds of >> expressions and instead handle everything we can. There's still >> the constant kind trees case which I'm too lazy to look into right >> now. I did refrain from adding SSA_NAME there and instead avoided >> calling set_mem_attributes_minus_bitpos when debug expression >> expansion ended up expanding a SSA definition RHS which should >> already have taken care of setting the appropriate MEM_ATTRS. > >Do we really need to ditch the entire ARRAY_REF path though? Aren't we >going >to lose some (trivial) disambiguation possibilities later on by >recording the >ARRAY_REF instead of DECL or COMPONENT_REF (+ offset)? The patch ends up recording the whole Array ref with variable index. All alias analysis code deals with this just fine. IIRC historically we tried to save memory with stripping and dropping of MEM_EXPRs. Richard.
