[PATCH] D31062: PR32288: Describe a bool parameter's DWARF location with a simple register

[David Blaikie via llvm-commits]

I /think/ this is legitimately unreachable, or probably should be. (but if
you could help with how to test it, if it is testable, I'd appreciate that)

The function is for converting a dbg.declare to a dbg.value. A dbg.declare
should never describe a fragment, should it? It's for describing the whole
that's stored in a static alloca, if I understand correctly?

On Fri, Mar 17, 2017 at 11:38 AM Adrian Prantl <aprantl at apple.com> wrote:

> On Mar 17, 2017, at 11:34 AM, David Blaikie <dblaikie at gmail.com> wrote:
>
> I suppose the question is: is it worth/necessary to provide a test case?
> We could include a fixme & not do anything here? Or I can work up an IR
> test case for IR Clang (& probably Swift) don't currently generate, so
> behavior that won't be used.
>
>
> IMO if the IR passes the Verifier, we should support it. And we didn't
> test other frontends, such as Julia, for example. If it is just a matter of
> making a small modification to IR generated by clang, I feel more
> comfortable with supporting it.
>
> -- adrian
>
>
> On Fri, Mar 17, 2017 at 11:15 AM Adrian Prantl <aprantl at apple.com> wrote:
>
> On Mar 17, 2017, at 9:12 AM, David Blaikie <dblaikie at gmail.com> wrote:
>
> Hmm, one wrinkle...
>
> the "if (auto Fragment ...)" case doesn't run in any existing test cases.
> (I put an "assert(false)" there & ran check-llvm).
>
> Any ideas if that's still reachable? Perhaps this only happens for direct
> bool parameters, never fragments.
>
>
> I just finished building the swift stdlib in 64 and 32 bit (which is
> typically a good source of these) and also didn't hit the assertion. We'd
> need to hand-craft a testcase for opt -mem2reg.
>
> -- adrian
>
>
> On Fri, Mar 17, 2017 at 8:28 AM Adrian Prantl via Phabricator <
> reviews at reviews.llvm.org> wrote:
>
> aprantl accepted this revision.
> aprantl added a comment.
> This revision is now accepted and ready to land.
>
> Took me a while to find the functional change amidst the refactoring :-)
> As discussed in the PR, this LGTM, thanks!
> One inline comment about a comment.
>
>
>
> ================
> Comment at: lib/Transforms/Utils/Local.cpp:1118
>      // describing will always be smaller than the variable size, because
>      // VariableSize == Size of Alloca described by DDI. Since SI stores
>      // to the alloca described by DDI, if it's first operand is an extend,
> ----------------
> Does this comment still match our understanding?
> Particularly the `The fragment we're describing will always be smaller
> than the variable size` bit.
>
>
> https://reviews.llvm.org/D31062
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170319/6addb89b/attachment.html>