This is an archive of the discontinued LLVM Phabricator instance.

DAG combiner: fold (select, C, X, undef) -> X
ClosedPublic

Authored by rampitec on Nov 16 2018, 12:54 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

rampitec created this revision.Nov 16 2018, 12:54 PM

This pretty much reproduces D48376, but you should copy the test I added

This pretty much reproduces 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?

This pretty much reproduces 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

This pretty much reproduces 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 D54606 to check both undef and non-undef cases 2) Commit D54606 first and then add select-undef.ll tests to this review which would use insertelement.

This pretty much reproduces 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 D54606 to check both undef and non-undef cases 2) Commit D54606 first and then add select-undef.ll tests to this review which would use insertelement.

I think I found one fp call with required combining, @llvm.amdgcn.rcp.f32.

I think this was to utilize dmask turning into undef:

if (!DMask && !BaseOpcode->Store) {
  // Eliminate no-op loads. Stores with dmask == 0 are *not* no-op: they
  // store the channels' default values.
  SDValue Undef = DAG.getUNDEF(Op.getValueType());
  if (isa<MemSDNode>(Op))
    return DAG.getMergeValues({Undef, Op.getOperand(0)}, DL);
  return Undef;

Any way that makes a non-obvious in the IR undef should work

rampitec updated this revision to Diff 174446.Nov 16 2018, 2:02 PM

Added test from D48376.

rampitec updated this revision to Diff 174454.Nov 16 2018, 2:38 PM

Added negative checks to select-undef.ll and rebased.

arsenm accepted this revision.Nov 16 2018, 3:08 PM

LGTM

This revision is now accepted and ready to land.Nov 16 2018, 3:08 PM
This revision was automatically updated to reflect the committed changes.