On Tue, Sep 29, 2020 at 9:23 AM Alexandre Oliva <ol...@adacore.com> wrote: > > On Sep 28, 2020, Richard Biener <richard.guent...@gmail.com> wrote: > > > On Fri, Sep 25, 2020 at 3:39 PM Alexandre Oliva <ol...@adacore.com> wrote: > > >> This patch introduces various improvements to the logic that merges > >> field compares. > > > Sorry for throwing a wrench in here but doing this kind of transform > > during GENERIC folding is considered a bad thing. > > Ugh. Even tree-ifcombine itself relies on tree folding logic to perform > this and various similar sorts of optimizations.
ifcombine should stop using fold*, yeah - there was a start last year making maybe_fold_and/or_comparisons rely on match.pd patterns instead but it didn't went all the way disabling dispatching to fold. I also think it will not end up using the simplifications using loads. > Is the issue just that we don't want to perform this kind of > transformation while still in GENERIC, or that the logic must not even > be there? I ask because it wouldn't be too hard to disable unwanted > cases of folding while in GENERIC, and extending it to follow SSA defs > so that ifcombine would those optimizations back at an acceptable spot. One of of the issues of these "combine two memory ops using $fancy for branches" transforms is that they are quite premature, making optimizations like SRA, value-range propagation, jump threading, etc. very much harder. Not to mention diagnostics that rely on the IL being close to the AST (well, I regularly argue those are broken expectations, so ;)) > Please help me understand what it is that makes this change as it is > undesirable, so that I can figure out how to do it in an acceptable way, > and justify the additional time and effort it will take. The desire is to move the existing and related transforms (optimize_bit_field_compare) to GIMPLE. By making them more powerful this task is made more difficult. Specifically your patch seems to introduce splitting of loads at alignment boundaries which is even more pemature since some targets would likely prefer an unaligned single load plus we try to not deviate too much in the initial phase of compilation depending on target capabilities. It also seems to be sth not suited for GENERIC which is somehow our highlevel AST. > I *think* ifcombine could even be extended so as to reuse the > separate-test logic I put in, by looking for non-immediate dominating > outer conditions for the inner condition. A further modified version of > fold_truth_andor_1 could then be used to combine the separate tests. I think the structure of ifcombine doesn't exactly match what fold_truth_andor does but since ifcombine first matches the if "structure" a common worker could be indeed factored out (but as said above, I see little value in retaining fold_truth_andor on GENERIC). > > As for reassoc... It doesn't seem a good fit at all for reassociating > short-circuiting boolean operations, that normally get translated as > multiple basic blocks. Yeah - it did accumulate quite some range test optimizations but I think it doesn't cover multiple BBs. Richard. > -- > Alexandre Oliva, happy hacker > https://FSFLA.org/blogs/lxo/ > Free Software Activist > GNU Toolchain Engineer
