Is it OK to remove metadata in an optimization pass? The context is patch http://reviews.llvm.org/D4571 which removes loop unrolling hint metadata after using it to avoid unrolling more than the hint suggests. This is a potential problem because loop unrolling can be run more than once. Example: a loop annotated with "#pragma clang loop unroll_count(2)" which adds hint metadata to the loop would be unrolled twice every time the loop unrolling pass is run. Anyway, I ask about metadata removal because Eli who is reviewing the patch wasn't sure whether this was acceptable. Loop unrolling metadata can take the following forms: llvm.loop.unroll.enable false // don't unroll llvm.loop.unroll.enable true // try to fully unroll llvm.loop.unroll.count N // try to unroll N times An alternative implementation to deleting nodes would be to just add a "llvm.loop.unroll.enable false" metadata node after unrolling. It's a little funny because you could then have, say, both "llvm.loop.unroll.enable false" and "llvm.loop.unroll.enable true" attached to a loop which is a bit funny. Yet another alternative is to make up some new metadata node type like "llvm.loop.unroll.already_unrolled" and add it to the loop. Mark -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20140717/1e7a68e1/attachment.html>
On Thu, Jul 17, 2014 at 11:48 PM, Mark Heffernan <meheff at google.com> wrote:> Is it OK to remove metadata in an optimization pass? The context is patch > http://reviews.llvm.org/D4571 which removes loop unrolling hint metadata > after using it to avoid unrolling more than the hint suggests. This is a > potential problem because loop unrolling can be run more than once. > Example: a loop annotated with "#pragma clang loop unroll_count(2)" which > adds hint metadata to the loop would be unrolled twice every time the loop > unrolling pass is run. Anyway, I ask about metadata removal because Eli who > is reviewing the patch wasn't sure whether this was acceptable. > > Loop unrolling metadata can take the following forms: > llvm.loop.unroll.enable false // don't unroll > llvm.loop.unroll.enable true // try to fully unroll > llvm.loop.unroll.count N // try to unroll N times > > An alternative implementation to deleting nodes would be to just add a > "llvm.loop.unroll.enable false" metadata node after unrolling. It's a > little funny because you could then have, say, both "llvm.loop.unroll.enable > false" and "llvm.loop.unroll.enable true" attached to a loop which is a bit > funny. Yet another alternative is to make up some new metadata node type > like "llvm.loop.unroll.already_unrolled" and add it to the loop.In general metadata is designed so it can be safely dropped without changing behavior of the resulting code. It can, however have an effect on the optimizer. IMO dropping it is also the right thing to do in your case as you fulfilled the request to unroll the loop. - Ben
> An alternative implementation to deleting nodes would be to just add a > "llvm.loop.unroll.enable false" metadata node after unrolling. It's a > little funny because you could then have, say, both "llvm.loop.unroll.enable > false" and "llvm.loop.unroll.enable true" attached to a loop which is a bit > funny. Yet another alternative is to make up some new metadata node type > like "llvm.loop.unroll.already_unrolled" and add it to the loop.What about dividing the unroll hint by the unroll count and dropping it if that's <= 1? Cheers. Tim.
On Fri, Jul 18, 2014 at 2:30 AM, Tim Northover <t.p.northover at gmail.com> wrote:> > An alternative implementation to deleting nodes would be to just add a > > "llvm.loop.unroll.enable false" metadata node after unrolling. It's a > > little funny because you could then have, say, both > "llvm.loop.unroll.enable > > false" and "llvm.loop.unroll.enable true" attached to a loop which is a > bit > > funny. Yet another alternative is to make up some new metadata node type > > like "llvm.loop.unroll.already_unrolled" and add it to the loop. > > What about dividing the unroll hint by the unroll count and dropping > it if that's <= 1? >This is a destructive change very similar to Mark's original in the patch. My issue with destructive changes is that they may make things more difficult to debug by "deleting the audit trail", so to say. But this isn't a strong objection, so if others feel it's appropriate in this case, I'm fine with that. Eli -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20140718/70e46722/attachment.html>
On Fri, Jul 18, 2014 at 2:30 AM, Tim Northover <t.p.northover at gmail.com> wrote:> What about dividing the unroll hint by the unroll count and dropping > it if that's <= 1? >That would usually get to an unroll factor closer to what the user requested, though most of the time they wouldn't be divisible so you'd rarely end up with the exact factor in the end. I'm a little on the fence about the compiler doing any unrolling if the pragma target can't be met. Maybe it should just emit a warning and give up. Currently it unrolls as much as possible if the target can't be met (it will emit a warning in this case once I finish a patch I'm working on). Though I can see bike-shedding arguments for either case. Mark> Cheers. > > Tim. >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20140718/1fe0f4e9/attachment.html>