This is an archive of the discontinued LLVM Phabricator instance.

Handle big index in getelementptr instruction
ClosedPublic

Authored by chfast on Mar 10 2015, 11:50 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

chfast updated this revision to Diff 21612.Mar 10 2015, 11:50 AM
chfast retitled this revision from to Handle big index in getelementptr instruction.
chfast updated this object.
chfast edited the test plan for this revision. (Show Details)
chfast set the repository for this revision to rL LLVM.
chfast added subscribers: Unknown Object (MLST), resistor, mcrosier.
rnk added a subscriber: rnk.Mar 10 2015, 12:56 PM
rnk added inline comments.
lib/CodeGen/SelectionDAG/FastISel.cpp
500 ↗(On Diff #21612)

We usually don't use auto when the type is non-obvious. uint64_t here would help readability.

521 ↗(On Diff #21612)

Don't shadow the outer 'I' variable.

521 ↗(On Diff #21612)

This sizeof computation doesn't get PtrSize. LLVM is a cross compiler. You can compute it out of the loop.

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
3432–3442 ↗(On Diff #21612)

Too much auto is bad for readability.

chfast added inline comments.Mar 10 2015, 1:19 PM
lib/CodeGen/SelectionDAG/FastISel.cpp
521 ↗(On Diff #21612)

I don't want a PtrSize, I want TotalOffs size in bits, because I'm going to add to that variable.

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
3432–3442 ↗(On Diff #21612)

Ok, I will change that, but I don't agree. From my point as a newbie it's better to have auto. Let's look at PtrTy variable: .getPointerTy() method returns value of type MVT and I have no idea that that mean. Moreover, in previous version it was converted implicitly to EVT. I don't care what C++ compiler type PtrTy has, I know it represents a pointer type (the name informs about that) and I want to know its size and create a constant of that type.

majnemer added inline comments.
lib/CodeGen/SelectionDAG/FastISel.cpp
521 ↗(On Diff #21612)

In that case, please just use 64. LLVM assumes that pointers are not wider than 64-bits.

Is this functionally different from:

CI->getValue().trunc(64).getSExtValue();

?

chfast added inline comments.Mar 10 2015, 1:55 PM
lib/CodeGen/SelectionDAG/FastISel.cpp
521 ↗(On Diff #21612)

It is different. CI->getValue() can be also smaller than i64, e.g i8. We need to sext if smaller, trunc if bigger.

chfast updated this revision to Diff 21715.Mar 11 2015, 7:15 AM

Suggestions from reviewers applied

rnk accepted this revision.Mar 11 2015, 1:50 PM
rnk added a reviewer: rnk.

lgtm, seems reasonable.

This revision is now accepted and ready to land.Mar 11 2015, 1:50 PM
This revision was automatically updated to reflect the committed changes.

Hi all,
Just be curious, is this fix to be platform-independent?
After I noticed the testcase is X86-only, I tested X86/getelementptr.ll with more platform+optimization level combinations. There are some crashes:

; RUN: llc < %s -O0 -march=arm64
; RUN: llc < %s -O0 -march=r600

For most of platforms -O2 seem fine, and no crash was found.

Is the result correct or not?

The backtrace of craches are attached below:
-march=arm64

llc: /home/ch/sources-llvm/llvm/include/llvm/ADT/APInt.h:1336: int64_t llvm::APInt::getSExtValue() const: Assertion `getMinSignedBits() <= 64 && "Too many bits for int64_t"' failed.
#0 0x1e5d41e llvm::sys::PrintStackTrace(llvm::raw_ostream&) /home/ch/sources-llvm/llvm/lib/Support/Unix/Signals.inc:424:15
#1 0x1e5e399 PrintStackTraceSignalHandler(void*) /home/ch/sources-llvm/llvm/lib/Support/Unix/Signals.inc:483:1
#2 0x1e5e873 SignalHandler(int) /home/ch/sources-llvm/llvm/lib/Support/Unix/Signals.inc:199:60
#3 0x7fc7b90468d0 __restore_rt (/lib/x86_64-linux-gnu/libpthread.so.0+0xf8d0)
#4 0x7fc7b8286107 gsignal /build/glibc-Ir_s5K/glibc-2.19/signal/../nptl/sysdeps/unix/sysv/linux/raise.c:56:0
#5 0x7fc7b82874e8 abort /build/glibc-Ir_s5K/glibc-2.19/stdlib/abort.c:91:0
#6 0x7fc7b827f226 __assert_fail_base /build/glibc-Ir_s5K/glibc-2.19/assert/assert.c:92:0
#7 0x7fc7b827f2d2 (/lib/x86_64-linux-gnu/libc.so.6+0x2e2d2)
#8 0x7bd18c llvm::APInt::getSExtValue() const /home/ch/sources-llvm/llvm/include/llvm/ADT/APInt.h:1337:20
#9 0x7d57fc llvm::ConstantInt::getSExtValue() const /home/ch/sources-llvm/llvm/include/llvm/IR/Constants.h:121:5
#10 0x8f4c57 (anonymous namespace)::AArch64FastISel::selectGetElementPtr(llvm::Instruction const*) /home/ch/sources-llvm/llvm/lib/Target/AArch64/AArch64FastISel.cpp:4848:13
#11 0x8b69bb (anonymous namespace)::AArch64FastISel::fastSelectInstruction(llvm::Instruction const*) /home/ch/sources-llvm/llvm/lib/Target/AArch64/AArch64FastISel.cpp:4962:5
#12 0x1c044d9 llvm::FastISel::selectInstruction(llvm::Instruction const*) /home/ch/sources-llvm/llvm/lib/CodeGen/SelectionDAG/FastISel.cpp:1350:7
#13 0x1d0ff12 llvm::SelectionDAGISel::SelectAllBasicBlocks(llvm::Function const&) /home/ch/sources-llvm/llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp:1250:13
#14 0x1d0e7ce llvm::SelectionDAGISel::runOnMachineFunction(llvm::MachineFunction&) /home/ch/sources-llvm/llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp:465:33
#15 0x80e083 (anonymous namespace)::AArch64DAGToDAGISel::runOnMachineFunction(llvm::MachineFunction&) /home/ch/sources-llvm/llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp:60:5
#16 0x15df0ce llvm::MachineFunctionPass::runOnFunction(llvm::Function&) /home/ch/sources-llvm/llvm/lib/CodeGen/MachineFunctionPass.cpp:40:3
#17 0x19b57bd llvm::FPPassManager::runOnFunction(llvm::Function&) /home/ch/sources-llvm/llvm/lib/IR/LegacyPassManager.cpp:1538:23
#18 0x19b5ac8 llvm::FPPassManager::runOnModule(llvm::Module&) /home/ch/sources-llvm/llvm/lib/IR/LegacyPassManager.cpp:1558:16
#19 0x19b6189 (anonymous namespace)::MPPassManager::runOnModule(llvm::Module&) /home/ch/sources-llvm/llvm/lib/IR/LegacyPassManager.cpp:1616:23
#20 0x19b5d7e llvm::legacy::PassManagerImpl::run(llvm::Module&) /home/ch/sources-llvm/llvm/lib/IR/LegacyPassManager.cpp:1723:16
#21 0x19b6631 llvm::legacy::PassManager::run(llvm::Module&) /home/ch/sources-llvm/llvm/lib/IR/LegacyPassManager.cpp:1756:3
#22 0x784e01 compileModule(char**, llvm::LLVMContext&) /home/ch/sources-llvm/llvm/tools/llc/llc.cpp:378:3
#23 0x783c76 main /home/ch/sources-llvm/llvm/tools/llc/llc.cpp:201:13
#24 0x7fc7b8272b45 __libc_start_main /build/glibc-Ir_s5K/glibc-2.19/csu/libc-start.c:321:0
#25 0x783994 _start (/home/ch/build-debug/bin/llc+0x783994)
Stack dump:
0.	Program arguments: /home/ch/build-debug/bin/llc getelementptr.ll -O0 -march=arm64 
1.	Running pass 'Function Pass Manager' on module 'getelementptr.ll'.
2.	Running pass 'AArch64 Instruction Selection' on function '@test_trunc65'

-march=r600

Not Implemented
UNREACHABLE executed at /home/ch/sources-llvm/llvm/lib/Target/R600/AMDGPUInstrInfo.cpp:97!
#0 0x1e5d41e llvm::sys::PrintStackTrace(llvm::raw_ostream&) /home/ch/sources-llvm/llvm/lib/Support/Unix/Signals.inc:424:15
#1 0x1e5e399 PrintStackTraceSignalHandler(void*) /home/ch/sources-llvm/llvm/lib/Support/Unix/Signals.inc:483:1
#2 0x1e5e873 SignalHandler(int) /home/ch/sources-llvm/llvm/lib/Support/Unix/Signals.inc:199:60
#3 0x7fc8ff8168d0 __restore_rt (/lib/x86_64-linux-gnu/libpthread.so.0+0xf8d0)
#4 0x7fc8fea56107 gsignal /build/glibc-Ir_s5K/glibc-2.19/signal/../nptl/sysdeps/unix/sysv/linux/raise.c:56:0
#5 0x7fc8fea574e8 abort /build/glibc-Ir_s5K/glibc-2.19/stdlib/abort.c:91:0
#6 0x1e0f746 (/home/ch/build-debug/bin/llc+0x1e0f746)
#7 0xf08af8 /home/ch/sources-llvm/llvm/lib/Target/R600/AMDGPUInstrInfo.cpp:97:3
#8 0x168a193 (anonymous namespace)::RAFast::spillVirtReg(llvm::MachineBasicBlock::bundle_iterator<llvm::MachineInstr, llvm::ilist_iterator<llvm::MachineInstr> >, (anonymous namespace)::RAFast::LiveReg*) /home/ch/sources-llvm/llvm/lib/CodeGen/RegAllocFast.cpp:290:5
#9 0x16883ab (anonymous namespace)::RAFast::spillAll(llvm::MachineBasicBlock::bundle_iterator<llvm::MachineInstr, llvm::ilist_iterator<llvm::MachineInstr> >) /home/ch/sources-llvm/llvm/lib/CodeGen/RegAllocFast.cpp:334:16
#10 0x168668a (anonymous namespace)::RAFast::AllocateBasicBlock() /home/ch/sources-llvm/llvm/lib/CodeGen/RegAllocFast.cpp:1067:3
#11 0x1684ba0 (anonymous namespace)::RAFast::runOnMachineFunction(llvm::MachineFunction&) /home/ch/sources-llvm/llvm/lib/CodeGen/RegAllocFast.cpp:1103:5
#12 0x15df0ce llvm::MachineFunctionPass::runOnFunction(llvm::Function&) /home/ch/sources-llvm/llvm/lib/CodeGen/MachineFunctionPass.cpp:40:3
#13 0x19b57bd llvm::FPPassManager::runOnFunction(llvm::Function&) /home/ch/sources-llvm/llvm/lib/IR/LegacyPassManager.cpp:1538:23
#14 0x19b5ac8 llvm::FPPassManager::runOnModule(llvm::Module&) /home/ch/sources-llvm/llvm/lib/IR/LegacyPassManager.cpp:1558:16
#15 0x19b6189 (anonymous namespace)::MPPassManager::runOnModule(llvm::Module&) /home/ch/sources-llvm/llvm/lib/IR/LegacyPassManager.cpp:1616:23
#16 0x19b5d7e llvm::legacy::PassManagerImpl::run(llvm::Module&) /home/ch/sources-llvm/llvm/lib/IR/LegacyPassManager.cpp:1723:16
#17 0x19b6631 llvm::legacy::PassManager::run(llvm::Module&) /home/ch/sources-llvm/llvm/lib/IR/LegacyPassManager.cpp:1756:3
#18 0x784e01 compileModule(char**, llvm::LLVMContext&) /home/ch/sources-llvm/llvm/tools/llc/llc.cpp:378:3
#19 0x783c76 main /home/ch/sources-llvm/llvm/tools/llc/llc.cpp:201:13
#20 0x7fc8fea42b45 __libc_start_main /build/glibc-Ir_s5K/glibc-2.19/csu/libc-start.c:321:0
#21 0x783994 _start (/home/ch/build-debug/bin/llc+0x783994)
Stack dump:
0.	Program arguments: /home/ch/build-debug/bin/llc getelementptr.ll -O0 -march=r600 
1.	Running pass 'Function Pass Manager' on module 'getelementptr.ll'.
2.	Running pass 'Fast Register Allocator' on function '@test_trunc65'

The fix was applied to generic CodeGen and is target independent. But it looks like the test case revealed bugs in target-specific code for some other targets. It looks like the crashes discovered are similar to the one I fixed - the assumption that GEP index is never bigger than 64 bit.

I can take a deeper look at that if you want.

The fix was applied to generic CodeGen and is target independent. But it looks like the test case revealed bugs in target-specific code for some other targets. It looks like the crashes discovered are similar to the one I fixed - the assumption that GEP index is never bigger than 64 bit.

I can take a deeper look at that if you want.

Hi chfast,

These revealed bugs seem probably unrelated with this fix, so we can just submit them into the bugzilla. But if it doesn't take you much time, please go ahead. Anyway, either is fine with me.

Cheers,
Chilledheart

The fix was applied to generic CodeGen and is target independent. But it looks like the test case revealed bugs in target-specific code for some other targets. It looks like the crashes discovered are similar to the one I fixed - the assumption that GEP index is never bigger than 64 bit.

I can take a deeper look at that if you want.

Hi chfast,

These revealed bugs seem probably unrelated with this fix, so we can just submit them into the bugzilla. But if it doesn't take you much time, please go ahead. Anyway, either is fine with me.

Cheers,
Chilledheart

arm64 needs the same fix, see D9140. R600 is different story, probably needs a bug report.