Page MenuHomePhabricator

Adding PoisonValue for representing poison value explicitly in IR
ClosedPublic

Authored by liuz on Dec 6 2019, 8:57 AM.

Details

Summary

Define ConstantData::PoisonValue.
Add support for poison value to LLLexer/LLParser/BitcodeReader/BitcodeWriter.
Add support for poison value to llvm-c interface.
Add support for poison value to OCaml binding.
Add m_Poison in PatternMatch.

Diff Detail

Event Timeline

liuz created this revision.Dec 6 2019, 8:57 AM
liuz edited the summary of this revision. (Show Details)Dec 6 2019, 9:18 AM
whitequark resigned from this revision.Mar 10 2020, 2:43 PM
RKSimon resigned from this revision.Aug 2 2020, 3:03 AM
liuz updated this revision to Diff 296566.Oct 6 2020, 5:21 PM
liuz retitled this revision from Add Poison Value to Adding PoisonValue for representing poison value explicitly in IR.Oct 6 2020, 5:32 PM
nikic added a reviewer: nikic.Oct 7 2020, 12:52 AM
nikic added a subscriber: nikic.

I would recommend making PoisonValue a subclass of UndefValue. Poison can always be relaxed to undef, and this will prevents lots of regressions when replacing undef with poison values.

aqjune added a comment.Oct 7 2020, 3:32 AM

I would recommend making PoisonValue a subclass of UndefValue. Poison can always be relaxed to undef, and this will prevents lots of regressions when replacing undef with poison values.

+1 for this. You'll need to update UndefValue::classof.

liuz updated this revision to Diff 297125.Oct 8 2020, 10:59 PM

PoisonValue is now a subclass of UndefValue

liuz edited the summary of this revision. (Show Details)Oct 8 2020, 11:00 PM
liuz updated this revision to Diff 307448.Nov 24 2020, 2:00 PM

rebased to upstream

reames accepted this revision.Nov 24 2020, 3:46 PM

LGTM.

p.s. I'm very unsure about the merits of having poison being a sub-class of undef, but I'm also completely fine landing this as is and adjusting later. Being able to spell poison directly in tests is a huge win, and I'm more than comfortable evolving in tree as warranted.

This revision is now accepted and ready to land.Nov 24 2020, 3:46 PM
This revision was landed with ongoing or failed builds.Nov 25 2020, 4:44 PM
This revision was automatically updated to reflect the committed changes.

http://lab.llvm.org:8011/#/builders/5/builds/1690/steps/9/logs/stdio

74536==WARNING: MemorySanitizer: use-of-uninitialized-value

#0 0x7c7b3ed in llvm::Value::setValueName(llvm::StringMapEntry<llvm::Value*>*) /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/lib/IR/Value.cpp:281:3
#1 0x7c78ec6 in destroyValueName /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/lib/IR/Value.cpp:143:3
#2 0x7c78ec6 in llvm::Value::~Value() /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/lib/IR/Value.cpp:104:3
#3 0x7b389e6 in operator() /b/sanitizer-x86_64-linux-fast/build/libcxx_build_msan/include/c++/v1/memory:2122:5
#4 0x7b389e6 in reset /b/sanitizer-x86_64-linux-fast/build/libcxx_build_msan/include/c++/v1/memory:2383:7
#5 0x7b389e6 in ~unique_ptr /b/sanitizer-x86_64-linux-fast/build/libcxx_build_msan/include/c++/v1/memory:2337:19
#6 0x7b389e6 in destroyAll /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/include/llvm/ADT/DenseMap.h:368:25
#7 0x7b389e6 in llvm::DenseMap<llvm::Type*, std::__1::unique_ptr<llvm::PoisonValue, std::__1::default_delete<llvm::PoisonValue> >, llvm::DenseMapInfo<llvm::Type*>, llvm::detail::DenseMapPair<llvm::Type*, std::__1::unique_ptr<llvm::PoisonValue, std::__1::default_delete<llvm::PoisonValue> > > >::~DenseMap() /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/include/llvm/ADT/DenseMap.h:754:11
#8 0x7b2f91d in llvm::LLVMContextImpl::~LLVMContextImpl() /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/lib/IR/LLVMContextImpl.cpp:125:1
#9 0x7b19cff in llvm::LLVMContext::~LLVMContext() /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/lib/IR/LLVMContext.cpp:94:31
#10 0x2561bb6 in main /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/tools/llc/llc.cpp:366:1
#11 0x7f93b5e3309a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2409a)
#12 0x24e0df9 in _start (/b/sanitizer-x86_64-linux-fast/build/llvm_build_msan/bin/llc+0x24e0df9)
cuviper added inline comments.
llvm/include/llvm-c/Core.h
272

This is a breaking change to the C ABI -- can we move it to the end of the enum?
https://bugs.llvm.org/show_bug.cgi?id=48905