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

[Dehao Chen via Phabricator via llvm-commits]

danielcdh added a comment.

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,
Dehao


https://reviews.llvm.org/D31900