This is an archive of the discontinued LLVM Phabricator instance.

Fix known zero bits for addrspacecast
ClosedPublic

Authored by yaxunl on Nov 17 2016, 9:18 AM.

Details

Summary

Currently LLVM assumes that a pointer addrspacecasted to a different addr space is equivalent to trunc or zext bitwise, which is not true. For example, in amdgcn target, when a null pointer is addrspacecasted from addr space 4 to 0, its value is changed from i64 0 to i32 -1.

This patch teaches LLVM not to assume known bits of addrspacecast instruction to its operand.

Diff Detail

Event Timeline

yaxunl updated this revision to Diff 78372.Nov 17 2016, 9:18 AM
yaxunl retitled this revision from to Fix known zero bits for addrspacecast.
yaxunl updated this object.
yaxunl added reviewers: tstellarAMD, arsenm.
yaxunl added a subscriber: llvm-commits.
yaxunl updated this revision to Diff 78373.Nov 17 2016, 9:21 AM

Simplify the test.

arsenm added inline comments.Nov 17 2016, 11:07 AM
lib/Analysis/ValueTracking.cpp
1028–1029

AddrSpaceCast should probably just be removed from the switch

yaxunl marked an inline comment as done.Nov 17 2016, 11:12 AM
yaxunl added inline comments.
lib/Analysis/ValueTracking.cpp
1028–1029

Some optimizations will be lost if it is removed.

arsenm added inline comments.Nov 17 2016, 11:13 AM
lib/Analysis/ValueTracking.cpp
1028–1029

Those optimizations are also broken

yaxunl marked an inline comment as done.Nov 17 2016, 11:14 AM
yaxunl added inline comments.
lib/Analysis/ValueTracking.cpp
1028–1029

But you are right. We cannot assume anything about the bits after addrspacecast.

yaxunl marked 3 inline comments as done.Nov 17 2016, 12:38 PM

we are going to lose alignment info after addrspacecast, e.g., the following test will fail:

; CHECK-LABEL: @static_hem_addrspacecast(
; CHECK: , align 16
define <2 x i64> @static_hem_addrspacecast() {
  %t = getelementptr <2 x i64>, <2 x i64>* @x, i32 7
  %t.asc = addrspacecast <2 x i64>* %t to <2 x i64> addrspace(1)*
  %tmp1 = load <2 x i64>, <2 x i64> addrspace(1)* %t.asc, align 1
  ret <2 x i64> %tmp1
}

I guess that's a price we need to pay for correctness.

yaxunl updated this revision to Diff 78404.Nov 17 2016, 12:46 PM
yaxunl updated this object.
mcrosier added inline comments.
test/Analysis/ValueTracking/knownzero-addrspacecast.ll
25

Please add a newline.

arsenm accepted this revision.Nov 17 2016, 2:09 PM
arsenm edited edge metadata.

LGTM. I think it should be ok to preserve the alignment though

This revision is now accepted and ready to land.Nov 17 2016, 2:09 PM

LGTM. I think it should be ok to preserve the alignment though

I think we may need another patch for that. Basically we may need a dedicated parameter for the known zero bits from alignment and pass it through addrspacecast.

test/Analysis/ValueTracking/knownzero-addrspacecast.ll
25

will do when committing.

This revision was automatically updated to reflect the committed changes.
yaxunl marked 2 inline comments as done.