On Thu, Dec 18, 2014 at 6:38 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. >> >> Thanks, >> Richard. >> > > Hi. > > There's second part of the patch set.
It still has similar issues: +/* Function compare for equality given memory operands T1 and T2. */ + +bool +func_checker::compare_memory_operand (tree t1, tree t2) +{ + if (!t1 && !t2) + return true; + else if (!t1 || !t2) + return false; + + if (TREE_THIS_VOLATILE (t1) != TREE_THIS_VOLATILE (t2)) + return return_false_with_msg ("different operand volatility"); + + bool source_is_memop = DECL_P (t1) || INDIRECT_REF_P (t1) + || TREE_CODE (t1) == MEM_REF + || TREE_CODE (t1) == TARGET_MEM_REF; + + bool target_is_memop = DECL_P (t2) || INDIRECT_REF_P (t2) + || TREE_CODE (t2) == MEM_REF + || TREE_CODE (t2) == TARGET_MEM_REF; + + /* Compare alias sets for memory operands. */ + if (source_is_memop && target_is_memop) + { + ao_ref r1, r2; + ao_ref_init (&r1, t1); + ao_ref_init (&r2, t2); the *_is_memop tests need to work on the memory reference base. Thus simply move the ao_ref_init's before those checks and do them on the result of ao_ref_base (). Similarly the volatile check can be put into the if (source_is_memop && target_is_memop) block, volatileness doesn't matter for non-memory operands. Ok with that changes. Thanks, Richard.