[PATCH] D28384: Update update_test_checks to work properly with phi nodes and other fun things.

[Daniel Berlin via llvm-commits]

On Fri, Jan 6, 2017 at 10:38 AM, bryant via Phabricator <
reviews at reviews.llvm.org> wrote:

> bryant added inline comments.
>
>
> ================
> Comment at: utils/update_test_checks.py:176
> +      if match.group(1) in vars_seen:
> +        line = line.replace('%' + match.group(1),
> get_value_use(match.group(1)), 1)
> +        continue
> ----------------
> bryant wrote:
> > bryant wrote:
> > > You're limiting the number of replaced uses to 1. Can a line have more
> than one use?
> > Just to clarify, what I meant is that you can do a lot better than
> manually replacing one at a time:
> >
> > ```
> > def genericize_check_lines(lines):
> >     def ssaify(match):
> >         v = match.group(1)
> >         if v in var_seen:
> >             rv = get_value_use(v)
> >         else:
> >             vars_seen.add('%' + v)
> >             rv = get_value_definition(v)
> >         return rv
> >     for line in lines:
> >         scrubbed_line = ...
> >         new_ine = IRVALUE_RE.sub(ssaify, scrubbed_line)
> >     ...
> > ```
> >
> > I suspect that underneath, `_sre` is building the replacement string
> incrementally, so it won't be quadratic.
> > Yes, but you want to replace one at a time anyway.
> >
> >
> > because it could be a use of "bb", and the line could be phi [a, bb],
> [b, bb17], [c, bb].
> >
> > If you replace  all uses of bb, you'll mistakenly replace bb17.
> >
> > We can do better, but not in this way (see my other email, this will
> generate wrong answers) , and it's IMHO not worth it.
> >
> > You can avoid doing the replacement directly by using the match start
> and end, but if you do, we either have to mess with the loop quite a bit.
> > Even if we stop using finditer (because we are replacing the string it
> is using as it goes, and it does not keep up to date) and use match
> directly, we have to offset where it starts looking for matches by the
> amount of text we've added by converting to var defs/uses, etc, or risk
> false matches.
> >
> >
> > None of this is worth it compared to keeping it simple like I did, imho.
>
>
Given how often we update testcases using it, still don't care.
But it seems to bug you, so i'll update the patch:)
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170106/3668f017/attachment.html>