Differential D129478
[IR] Remove support for float binop constant expressions Authored by nikic on Jul 11 2022, 4:29 AM.
Details As part of https://discourse.llvm.org/t/rfc-remove-most-constant-expressions/63179, this removes support for the floating-point binop constant expressions fadd, fsub, fmul, fdiv and frem. As part of this change, the C APIs LLVMConstFAdd, LLVMConstFSub, LLVMConstFMul, LLVMConstFDiv and LLVMConstFRem are removed. The LLVMBuild APIs should be used instead.
Diff Detail
Event TimelineComment Actions After this patch, llvm-dis will crash if passed a bitcode file with metadata that is now non-representable. The problem can be reproduced by running an llvm-as built before this patch on this .ll file, and then running the resulting bitcode file through an llvm-dis binary built after this patch. It crashes with an assertion failure: assertion failed at third_party/llvm/llvm-project/llvm/lib/IR/Metadata.cpp:393 in static llvm::ValueAsMetadata *llvm::ValueAsMetadata::get(llvm::Value *): V && "Unexpected null Value" and the following stack trace: Stack dump: 0. Program arguments: blaze-bin/third_party/llvm/llvm-project/llvm/llvm-dis /tmp/blah.bc #0 0x000055d0d798da2e llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) (blaze-bin/third_party/llvm/llvm-project/llvm/llvm-dis+0x58da2e) #1 0x000055d0d798df23 PrintStackTraceSignalHandler(void*) Signals.cpp:0:0 #2 0x000055d0d798e0c4 SignalHandler(int) Signals.cpp:0:0 #3 0x00007f9c9175a1c0 __restore_rt (/usr/grte/v5/lib64/libpthread.so.0+0x151c0) #4 0x00007f9c915ff347 gsignal (/usr/grte/v5/lib64/libc.so.6+0x75347) #5 0x00007f9c91600797 abort (/usr/grte/v5/lib64/libc.so.6+0x76797) #6 0x000055d0d7ad6071 absl::log_internal::LogMessage::SendToLog() (blaze-bin/third_party/llvm/llvm-project/llvm/llvm-dis+0x6d6071) #7 0x000055d0d7ad57e9 absl::log_internal::LogMessage::Flush() (blaze-bin/third_party/llvm/llvm-project/llvm/llvm-dis+0x6d57e9) #8 0x000055d0d7ad6439 absl::log_internal::LogMessageFatal::~LogMessageFatal() (blaze-bin/third_party/llvm/llvm-project/llvm/llvm-dis+0x6d6439) #9 0x000055d0d7a95454 __assert_fail (blaze-bin/third_party/llvm/llvm-project/llvm/llvm-dis+0x695454) #10 0x000055d0d783e52b llvm::ValueAsMetadata::get(llvm::Value*) (blaze-bin/third_party/llvm/llvm-project/llvm/llvm-dis+0x43e52b) #11 0x000055d0d76f995b llvm::MetadataLoader::MetadataLoaderImpl::parseOneMetadata(llvm::SmallVectorImpl<unsigned long>&, unsigned int, (anonymous namespace)::(anonymous namespace)::PlaceholderQueue&, llvm::StringRef, unsigned int&) MetadataLoader.cpp:0:0 #12 0x000055d0d76f5c0b llvm::MetadataLoader::MetadataLoaderImpl::parseMetadata(bool) (blaze-bin/third_party/llvm/llvm-project/llvm/llvm-dis+0x2f5c0b) #13 0x000055d0d76fe1c9 llvm::MetadataLoader::parseMetadata(bool) (blaze-bin/third_party/llvm/llvm-project/llvm/llvm-dis+0x2fe1c9) #14 0x000055d0d76c67b4 (anonymous namespace)::BitcodeReader::parseFunctionBody(llvm::Function*) BitcodeReader.cpp:0:0 #15 0x000055d0d76c2f90 (anonymous namespace)::BitcodeReader::materialize(llvm::GlobalValue*) BitcodeReader.cpp:0:0 #16 0x000055d0d76c3c9a (anonymous namespace)::BitcodeReader::materializeModule() BitcodeReader.cpp:0:0 #17 0x000055d0d785abbd llvm::Module::materializeAll() (blaze-bin/third_party/llvm/llvm-project/llvm/llvm-dis+0x45abbd) #18 0x000055d0d76b6e74 main (blaze-bin/third_party/llvm/llvm-project/llvm/llvm-dis+0x2b6e74) #19 0x00007f9c915eb633 __libc_start_main (/usr/grte/v5/lib64/libc.so.6+0x61633) #20 0x000055d0d76b652a _start (blaze-bin/third_party/llvm/llvm-project/llvm/llvm-dis+0x2b652a) Comment Actions @jgorbe Thanks for the report! To start with, I landed a change to make this report an error instead of crash: https://github.com/llvm/llvm-project/commit/159feac1c0a26dccc6dd8726bd71d073cda5faea I'm a bit unsure how to handle the metadata situation further. Actually expanding the constant expression sounds like quite the PITA because metadata is read before the function, so we wouldn't know where to insert the instructions at that point. As debuginfo auto-upgrade is not supported (the "upgrade" action for debuginfo is to strip it completely), I'm inclined to simply replace the metadata with an undef or poison value (which llvm.debug.value handles fine). Comment Actions Note that this change (and related changes) is nuking a huge amount of non-deprecated C API functions. It suggests using the LLVMBuild* versions, but those do not work without instantiating a builder, which typically does not exist on the global level. Allowing the user of the API to pass in a NULL builder would have made it more palatable, but as is the option is either to create a dummy builder for the global level (which isn't actually used! It's just to satisfy the contract of the LLVMBuild* functions) or try to do constant folding in the frontend. In addition the removal it also breaks all LLVM-C based bridges in other languages. To put it lightly, this change is not good. Unlike the migration to opaque pointers, which was handled with a long term deprecation mechanism, these changes to the API are both unexpected and seemingly unnecessary as there is no real need to remove them other than as a pedantic cleanup. |