On Thu, Dec 18, 2014 at 2:39 PM, Martin Liška <mli...@suse.cz> wrote: > On 12/17/2014 04:23 PM, Richard Biener wrote: >> >> On Wed, Dec 17, 2014 at 12:17 PM, Martin Liška <mli...@suse.cz> wrote: >>> >>> On 12/11/2014 01:37 PM, Richard Biener wrote: >>>> >>>> >>>> On Wed, Dec 10, 2014 at 1:18 PM, Martin Liška <mli...@suse.cz> wrote: >>>>> >>>>> >>>>> Hello. >>>>> >>>>> As suggested by Richard, I split compare_operand functions to various >>>>> functions >>>>> related to a specific comparison. Apart from that I added fast check >>>>> for >>>>> volatility flag that caused miscompilation mentioned in PR63569. >>>>> >>>>> Patch can bootstrap on x86_64-linux-pc without any regression seen and >>>>> I >>>>> was >>>>> able to build Firefox with LTO. >>>>> >>>>> Ready for trunk? >>>> >>>> >>>> >>>> Hmm, I don't think the dispatch to compare_memory_operand is at the >>>> correct place. It should be called from places where currently >>>> compare_operand is called and it should recurse to compare_operand. >>>> That is, it is more "high-level". >>>> >>>> Can you please fix the volatile issue separately? It's also not >>>> necessary >>>> to do that check on every operand but just on memory operands. >>> >>> >>> >>> Hello Richard. >>> >>> I hope the following patch is the finally the right approach we want to >>> do >>> ;) >>> In the first patch, I did just refactoring for high-level memory operand >>> comparison and the second of fixes problem connected to missing volatile >>> attribute comparison. >>> >>> Patch was retested and can bootstrap. >>> >>> What do you think about it? >> >> >> +bool >> +func_checker::compare_cst_or_decl (tree t1, tree t2) >> +{ >> ... >> + case INTEGER_CST: >> + { >> + ret = compatible_types_p (TREE_TYPE (t1), TREE_TYPE (t2)) >> + && wi::to_offset (t1) == wi::to_offset (t2); >> + >> + return return_with_debug (ret); >> + } >> + case COMPLEX_CST: >> + case VECTOR_CST: >> + case STRING_CST: >> + case REAL_CST: >> + { >> + ret = operand_equal_p (t1, t2, OEP_ONLY_CONST); >> + return return_with_debug (ret); >> >> why does the type matter for INTEGER_CSTs but not for other constants? >> Also comparing INTEGER_CSTs via to_offset is certainly wrong. Please >> use tree_int_cst_equal_p (t1, t2) instead, or operand_equal_p which would >> end up calling tree_int_cst_equal_p. That said, I'd have commonized all >> _CST nodes by >> >> ret = compatible_types_p (....) && operand_equal_p (...); >> >> + case CONST_DECL: >> + case BIT_FIELD_REF: >> + { >> >> I'm quite sure BIT_FIELD_REF is spurious here. >> >> Now to the "meat" ... >> >> + tree load1 = get_base_loadstore (t1, false); >> + tree load2 = get_base_loadstore (t2, false); >> + >> + if (load1 && load2 && !compare_memory_operand (t1, t2)) >> + return return_false_with_msg ("memory operands are different"); >> + else if ((load1 && !load2) || (!load1 && load2)) >> + return return_false_with_msg (""); >> + >> >> and the similar code for assigns. The way you introduce the >> unpack_handled_component flag to get_base_loadstore makes >> it treat x.field as non-memory operand. That's wrong. I wonder >> why you think you needed that. Why does >> >> tree load1= get_base_loadstore (t1); >> >> not work? Btw, I'd have avoided get_base_loadstore and simply did >> >> ao_ref_init (&r1, t1); >> ao_ref_init (&r2, t2); >> tree base1 = ao_ref_base (&r1); >> tree base2 = ao_ref_base (&r2); >> if ((DECL_P (base1) || (TREE_CODE (base1) == MEM_REF || TREE_CODE >> (base1) == TARGET_MEM_REF)) >> && (DECL_P (base2) || (...)) >> ... >> >> or rather put this into compare_memory_operand and call that on all >> operands >> that may be memory (TREE_CODE () != SSA_NAME && !is_gimple_min_invariant >> ()). >> >> I also miss where you handle memory operands on the LHS for calls >> and for assignments. >> >> The compare_ssa_name refactoring part is ok to install. >> >> Can you please fix the volatile bug before the refactoring as it looks >> like >> we're going to iterate some more here unless I can find the time to give >> it a quick try myself. > > > Hi. > > Sure, there's minimalistic patch which fixes the PR. > Can I install this change?
Ok. Thanks, Richard. > I'm going to apply your notes that are orthogonal to the PR. > > Thanks, > Martin > > >> >> Thanks, >> Richard. >> >>> Thank you, >>> Martin >>> >>>> >>>> Thanks, >>>> Richard. >>>> >>>>> Thanks, >>>>> Martin >>> >>> >>> >