[PATCH] D54646: DAG combiner: fold (select, C, X, undef) -> X

[Stanislav Mekhanoshin via Phabricator via llvm-commits]

rampitec added a comment.

In https://reviews.llvm.org/D54646#1301593, @arsenm wrote:

> In https://reviews.llvm.org/D54646#1301582, @rampitec wrote:
>
> > In https://reviews.llvm.org/D54646#1301556, @arsenm wrote:
> >
> > > This pretty much reproduces https://reviews.llvm.org/D48376, but you should copy the test I added
> >
> >
> > The test you have added there does not work anymore.
> >  What's the new syntax for @llvm.amdgcn.image.sample.f32.v2f32.v8i32? Is it relevant to use image sample in this test at all?
>
>
> I think it's because it avoids the fact that regular loads are converted to integer loads, and it was to ensure it was an FP type. You should be able to just copy any other FP returning intrinsic use from another test


Matt, I do not understand your test. I believe it does not test what's intended. For some reason your checks are:

  ; GCN: s_waitcnt
  ; GCN-NEXT: s_setpc_b64

I.e. you seem to check there is no v_cndmask_b32, though it is never checked. But it looks like you believe that an intrinsic call with undef operands will be combined into undef. This is not the case most of the times and not even a case for image sample anymore. This is almost the test for image_sample(undef) folding, not select(undef).

I understand the point of the test, just do not believe it is correct. I see two possible ways: 1) commit this as is and extend tests from https://reviews.llvm.org/D54606 to check both undef and non-undef cases 2) Commit https://reviews.llvm.org/D54606 first and then add select-undef.ll tests to this review which would use insertelement.


https://reviews.llvm.org/D54646