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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

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).

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).


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

https://reviews.llvm.org/D88684