[PATCH] D88684: llvm-reduce: Don't replace intrinsic calls with undef

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added a comment.

In D88684#2328377 <https://reviews.llvm.org/D88684#2328377>, @dblaikie wrote:

> In D88684#2327661 <https://reviews.llvm.org/D88684#2327661>, @arsenm wrote:
>
>> In D88684#2318436 <https://reviews.llvm.org/D88684#2318436>, @ychen wrote:
>>
>>> In D88684#2318429 <https://reviews.llvm.org/D88684#2318429>, @dblaikie wrote:
>>>
>>>> In D88684#2318361 <https://reviews.llvm.org/D88684#2318361>, @arsenm wrote:
>>>>
>>>>> In D88684#2318108 <https://reviews.llvm.org/D88684#2318108>, @ychen wrote:
>>>>>
>>>>>> The test tieing to AMDGPU seems less ideal for general functionality. I've seen llvm.dbg.value caused the same problem, maybe we could use that?
>>>>>
>>>>> The debug intrinsics require a *lot* of metadata overhead simply to satsify the operations. The other metadata using intrinsics are experimental, so this seemed like the least fragile way to test it
>>>>
>>>> Given the functionality isn't implemented in terms of metadata-using-intrinsics in particular, a test using some non-metadata-using intrinsic would be fine.
>>>
>>> I think only the callee matters here. If anyone makes changes to llvm-reduce and not build AMDGPU, there is a chance (should be small I guess) to regress this.
>>
>> Developers should just always build all targets. We also have buildbots for this
>
> Not necessarily - https://llvm.org/docs/GettingStarted.html suggests narrowing the target list in a few places, to speed up iteration time. So making tests more portable across targets is valuable - also means buildbots will get to your tests sooner (as the test will be run on more configurations, increasing the chances it'll be run on a buildbot that's running faster/picking up your changes sooner).

I disagree with this (maybe it's OK for frontend work), and you should run all target tests before committing backend changes. For this test it doesn't actually require the target so I've just dropped the REQUIRES

> In D88684#2327660 <https://reviews.llvm.org/D88684#2327660>, @arsenm wrote:
>
>> In D88684#2318429 <https://reviews.llvm.org/D88684#2318429>, @dblaikie wrote:
>>
>>> In D88684#2318361 <https://reviews.llvm.org/D88684#2318361>, @arsenm wrote:
>>>
>>>> In D88684#2318108 <https://reviews.llvm.org/D88684#2318108>, @ychen wrote:
>>>>
>>>>> The test tieing to AMDGPU seems less ideal for general functionality. I've seen llvm.dbg.value caused the same problem, maybe we could use that?
>>>>
>>>> The debug intrinsics require a *lot* of metadata overhead simply to satsify the operations. The other metadata using intrinsics are experimental, so this seemed like the least fragile way to test it
>>>
>>> Given the functionality isn't implemented in terms of metadata-using-intrinsics in particular, a test using some non-metadata-using intrinsic would be fine.
>>
>> I'm specifically checking for the invalid intrinsic error produced. I think another intrinsic would be less clear for what is actually happening
>
> Ah, please tweak the test to test the general behavior (not modifying intrinsic calls), not the invalidity/second order that lead to the root cause/change in behavior (that some of those modified intrinsic calls then become invalid).

I don't see how else I would tell from the output that the intrinsic call itself is why it was skipped. The messages printed don't directly indicate why anything was skipped. The interestingness script needs to keep the call around


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88684/new/

https://reviews.llvm.org/D88684