Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Avoid migrating existing patches. Phabricator shutdown timeline

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