This is an archive of the discontinued LLVM Phabricator instance.

Fix APInt bit size in processDbgDeclares
ClosedPublic

Authored by yaxunl on Nov 15 2017, 8:59 AM.

Details

Summary

processDbgDeclares assumes pointer size is the same for different addr spaces.
It uses pointer size for addr space 0 for all pointers, which causes assertion
in stripAndAccumulateInBoundsConstantOffsets for amdgcn---amdgiz since
pointer in addr space 5 has different size than in addr space 0.

This patch fixes that.

Diff Detail

Repository
rL LLVM

Event Timeline

yaxunl created this revision.Nov 15 2017, 8:59 AM
This revision is now accepted and ready to land.Nov 15 2017, 11:47 AM
arsenm added inline comments.Nov 15 2017, 12:10 PM
lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
1147 ↗(On Diff #123037)

Is a vector of pointers allowed here? How could this never be a PointerType?

yaxunl added inline comments.Nov 15 2017, 1:29 PM
lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
1147 ↗(On Diff #123037)

There are lit tests in which Address is not pointer, e.g. CodeGen/X86/selectiondag-order.ll

define i64 @simulateWithDbgDeclare(<2 x i32> %a) local_unnamed_addr  {
entry:
  %rand = tail call i64 @lrand48() #3
  tail call void @llvm.dbg.declare(metadata i64 %rand, metadata !6, metadata !7), !dbg !8
  br label %body
arsenm edited edge metadata.Nov 15 2017, 3:32 PM

If this allows non-pointers, I’m very suspicious of using a pointer size for this of any address space. Something seems off about this. Should it use some maximum size int type?

Should it just be the type size? There seems like no reason to use the low level pointer size query

Should it just be the type size? There seems like no reason to use the low level pointer size query

That sounds reasonable. Will do.

yaxunl updated this revision to Diff 123121.Nov 15 2017, 6:34 PM

Revised by Matt's comments.

arsenm accepted this revision.Nov 15 2017, 6:38 PM

LGTM

This revision was automatically updated to reflect the committed changes.