Page MenuHomePhabricator

[X86] Fix chains update when lowering BUILD_VECTOR to a vector load
ClosedPublic

Authored by apilipenko on Oct 4 2017, 9:44 AM.

Details

Summary

The code which lowers BUILD_VECTOR of consecutive loads into a single vector load doesn't update chains properly. As a result the vector load can be reordered with the store to the same location.

Consider the test case.

define <4 x i32> @merge_4i32_i32_23u5_inc3(i32* %ptr) nounwind uwtable noinline ssp {
  %ptr0 = getelementptr inbounds i32, i32* %ptr, i64 2
  %ptr1 = getelementptr inbounds i32, i32* %ptr, i64 3
  %ptr3 = getelementptr inbounds i32, i32* %ptr, i64 5
  %val0 = load i32, i32* %ptr0
  %val1 = load i32, i32* %ptr1
  %inc = add i32 %val1, 1
  store i32 %inc, i32* %ptr1
  %val3 = load i32, i32* %ptr3
  %res0 = insertelement <4 x i32> undef, i32 %val0, i32 0
  %res1 = insertelement <4 x i32> %res0, i32 %val1, i32 1
  %res3 = insertelement <4 x i32> %res1, i32 %val3, i32 3
  ret <4 x i32> %res3
}

llc -mtriple=x86_64-unknown-unknown -mattr=+sse2 -debug-only=isel

Type-legalized selection DAG: BB#0 'merge_4i32_i32_23u5_inc3:'

SelectionDAG has 22 nodes:
  t0: ch = EntryToken
  t2: i64,ch = CopyFromReg t0, Register:i64 %vreg0
  t6: i64 = add t2, Constant:i64<12>
  t11: i32,ch = load<LD4[%ptr1]> t0, t6, undef:i64
      t13: i32 = add t11, Constant:i32<1>
    t14: ch = store<ST4[%ptr1]> t11:1, t13, t6, undef:i64
        t4: i64 = add t2, Constant:i64<8>
      t35: i32,ch = load<LD4[%ptr0]> t0, t4, undef:i64
        t8: i64 = add t2, Constant:i64<20>
      t33: i32,ch = load<LD4[%ptr3]> t0, t8, undef:i64
    t32: v4i32 = BUILD_VECTOR t35, t11, undef:i32, t33
  t27: ch,glue = CopyToReg t14, Register:v4i32 %XMM0, t32
  t28: ch = X86ISD::RET_FLAG t27, TargetConstant:i32<0>, Register:v4i32 %XMM0, t27:1

Legalized selection DAG: BB#0 'merge_4i32_i32_23u5_inc3:'

SelectionDAG has 18 nodes:
  t0: ch = EntryToken
  t2: i64,ch = CopyFromReg t0, Register:i64 %vreg0
  t6: i64 = add t2, Constant:i64<12>
  t11: i32,ch = load<LD4[%ptr1]> t0, t6, undef:i64
      t13: i32 = add t11, Constant:i32<1>
    t14: ch = store<ST4[%ptr1]> t11:1, t13, t6, undef:i64
        t4: i64 = add t2, Constant:i64<8>
      t38: v2i64,ch = load<LD16[%ptr0](align=4)> t0, t4, undef:i64
    t39: v4i32 = bitcast t38
  t27: ch,glue = CopyToReg t14, Register:v4i32 %XMM0, t39
  t28: ch = X86ISD::RET_FLAG t27, TargetConstant:i32<0>, Register:v4i32 %XMM0, t27:1

In the last DAG there are two chains:
t0 - t11 (scalar load) - t14 (scalar store)
t0 - t38 (vector load)

There is no ordering constraint between t38 (vector load) and t14 (scalar store), so they can be reordered. That's what happens in the final assembly:

# BB#0:
  incl  12(%rdi)
  movups  8(%rdi), %xmm0
  retq

Here the scalar store happens before the vector load. The vector load reloads the incremented value for 12(%rdi) location, while in the original program it uses the pre-increment value.

The current code in EltsFromConsecutiveLoads tries to update the chains but fails becuase it only updates the chain following the first load. Had the incremented location been the first load we wouldn't miscompile. The fix is to update the chains following all the loads comprising the vector.

Diff Detail

Repository
rL LLVM

Event Timeline

apilipenko created this revision.Oct 4 2017, 9:44 AM
RKSimon edited edge metadata.Oct 4 2017, 9:48 AM

Is this PR10114 ?

Is this PR10114 ?

The problem was found in our internal testing. But it looks like PR10114 describes the same problem.

niravd accepted this revision.Oct 4 2017, 10:42 AM

This is the same fix as in D18663 (which is now way out of date) but with a test case! This should be all of PR10114 and we can finally mark it fixed.

LGTM.

This revision is now accepted and ready to land.Oct 4 2017, 10:42 AM
This revision was automatically updated to reflect the committed changes.