[PATCH] D31900: [AddDiscriminators] Assign discriminators to memset/memcpy/memmove intrinsic calls.

[Andrea Di Biagio via Phabricator via llvm-commits]

andreadb added a comment.

In https://reviews.llvm.org/D31900#723835, @andreadb wrote:

> In https://reviews.llvm.org/D31900#723761, @danielcdh wrote:
>
> > Thanks for the explanation. This makes sense. The reasons that we try to avoid IntrinsicInst is 2 fold:
> >
> > 1. avoid non-deterministic discriminator assignment for different debug level
> > 2. minimize the number of base discriminators
> >
> >   Apparently, bypassing all IntrinsicInst is an overkill to solve #1, and in fact it will cause the bug as shown in the unittest, and your patch managed to fix it. But for #2, your patch introduces extra base discriminators that is unnecessary. I suggest only keep the first callsite of shouldHaveDiscriminator, and revert the 2nd callsite to just check IntrinsicInst. Please add comment at both places to explain what's the motivation (the first place, i.e. call to shouldHaveDiscriminator is aiming at addressing #1, the 2nd place, i.e. checking IntrinsicInst, is aiming at addressing #1 and #2). And also in the unittest, please check explicitly that the discriminator for the memcpy intrinsic is the same as other instructions in that BB.
>
>
> Thanks for the feedback.
>  I will add the comments you have requested and I will restore the 2nd callsite to the check for IntrinsicInst.
>
> Do you want me to keep test memcpy-discriminator.ll in the final patch?
>
> About the unit test: is there a template that I can use for this particular case? I never had to write an llvm unittest before, so I don't know where to look at for examples.
>
> Thanks,
> Andrea


Just to avoid misunderstandings, when you say unittest do you mean my original `memcpy-discriminator.ll`?


https://reviews.llvm.org/D31900