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. > Thank you, > Martin > >> >> Thanks, >> Richard. >> >>> Thanks, >>> Martin > >