On Fri, Feb 10, 2012 at 4:36 PM, William J. Schmidt <wschm...@linux.vnet.ibm.com> wrote: > > > On Fri, 2012-02-10 at 16:22 +0100, Richard Guenther wrote: >> On Fri, Feb 10, 2012 at 3:39 PM, William J. Schmidt >> <wschm...@linux.vnet.ibm.com> wrote: >> > Jakub, thanks! Based on this, I believe the patch is correct in its >> > handling of the STMT_VINFO_PATTERN_DEF_SEQ logic, without any double >> > counting. >> > >> > I misinterpreted what the commentary for vect_pattern_recog was saying: >> > I thought that all replacements were hung off of the last pattern >> > statement, but this was just true for the particular example, where only >> > one replacement statement was created. Sorry for any confusion! >> > >> > Based on the discussion, is the patch OK for trunk? >> >> But you still count for each of the matched scalar stmts the cost of the >> whole pattern sequence. No? > > I need to change the commentary to make this more clear now. My > comment: "Translate the last statement..." is incorrect; this is done > for each statement in a pattern. > > Per Jakub's explanation, the replacement statements are distributed over > the original pattern statements.
Ok, looking at the code in tree-vect-patterns.c it seems that STMT_VINFO_IN_PATTERN_P is only set for the stmt that contains the first replacement stmt in STMT_VINFO_RELATED_STMT and further stmts in that stmts STMT_VINFO_PATTERN_DEF_SEQ. Other stmts that were matched do not have STMT_VINFO_IN_PATTERN_P set (but their STMT_VINFO_RELATED_STMT points to the stmt that has STMT_VINFO_IN_PATTERN_P set). Possibly the other scalar stmts participating in the pattern are marked as not relevant (couldn't spot this quickly). So it seems that your patch should indeed work as-is. Thus, ok, if Jakub doesn't spot another error. Thanks, Richard. > Visiting STMT_VINFO_RELATED_STMT for a > statement marked STMT_VINFO_IN_PATTERN_P will find zero or one > replacement statements that should be examined. Visiting > STMT_VINFO_PATTERN_DEF_SEQ may pick up some leftover replacement > statements that didn't fit in the 1-1 mapping. The point is that all of > these related statements and pattern-def-seq statements are disjoint, > and Ira's logic is ensuring that they all get examined once. > > It's not a very clean way to represent the replacement of a pattern -- > not how you'd do it if designing from scratch -- but I guess I can see > how it got this way when the STMT_VINFO_PATTERN_DEF_SEQ was glued onto > the existing 1-1 mapping. > > Bill > >> >> Richard. >> >> > Thanks, >> > Bill >> > >> > On Fri, 2012-02-10 at 14:44 +0100, Jakub Jelinek wrote: >> >> On Fri, Feb 10, 2012 at 07:31:01AM -0600, William J. Schmidt wrote: >> >> > >From the commentary at the end of tree-vect-patterns.c, only the main >> >> > statement in the pattern (the last one) is flagged as >> >> > STMT_VINFO_IN_PATTERN_P. So this is finding the new replacement >> >> > statement which has been created but not yet inserted in the code. It >> >> > only gets counted once. >> >> >> >> STMT_VINFO_IN_PATTERN_P is set on the vinfo of the original stmt (and not >> >> just the last, but all original stmts that are being replaced by the >> >> pattern). Each of these has STMT_VINFO_RELATED_STMT set to a pattern stmt >> >> (last one corresponding to that original stmt). If needed, further >> >> pattern stmts for that original stmts are put into >> >> STMT_VINFO_PATTERN_DEF_SEQ. The pattern matcher could e.g. match >> >> 3 original stmts, and stick a single pattern stmt into >> >> STMT_VINFO_RELATED_STMT >> >> on the first original stmt, then say 2 pattern stmts into >> >> STMT_VINFO_PATTERN_DEF_SEQ of the second original stmt plus one >> >> STMT_VINFO_RELATED_STMT and finally one pattern stmt through >> >> STMT_VINFO_RELATED_STMT on the third original stmt. >> >> >> >> > Note that STMT_VINFO_PATTERN_DEF_SEQ doesn't exist in the 4.6 branch, so >> >> > this section has to be omitted if we backport it (which is desirable >> >> > since the degradation was introduced in 4.6). Removing it apparently >> >> > does not affect the sphinx3 degradation. >> >> >> >> Yeah, when backporting you can basically assume that >> >> STMT_VINFO_PATTERN_DEF_SEQ is always NULL in 4.6 and just write NULL >> >> instead >> >> of itt (and simplify), because no pattern recognizers needed more than one >> >> pattern stmt for each original stmt back then. >> >> >> >> Jakub >> >> >> > >> >
