Page MenuHomePhabricator

[IR] Remove support for float binop constant expressions
ClosedPublic

Authored by nikic on Jul 11 2022, 4:29 AM.

Details

Summary

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

Unit TestsFailed

TimeTest
60,040 msx64 debian > MLIR.Examples/standalone::test.toy
Script: -- : 'RUN: at line 1'; /usr/bin/cmake /var/lib/buildkite-agent/builds/llvm-project/mlir/examples/standalone -G "Ninja" -DCMAKE_CXX_COMPILER=/usr/bin/clang++ -DCMAKE_C_COMPILER=/usr/bin/clang -DLLVM_ENABLE_LIBCXX=OFF -DMLIR_DIR=/var/lib/buildkite-agent/builds/llvm-project/build/lib/cmake/mlir -DLLVM_USE_LINKER=lld
60,060 msx64 debian > ThreadSanitizer-x86_64.ThreadSanitizer-x86_64::restore_stack.cpp
Script: -- : 'RUN: at line 1'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang --driver-mode=g++ -fsanitize=thread -Wall -m64 -msse4.2 -gline-tables-only -I/var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/tsan/../ -std=c++11 -I/var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/tsan/../ -O1 /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/tsan/restore_stack.cpp -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/tsan/X86_64Config/Output/restore_stack.cpp.tmp && not /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/tsan/X86_64Config/Output/restore_stack.cpp.tmp 2>&1 | FileCheck /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/tsan/restore_stack.cpp

Event Timeline

nikic created this revision.Jul 11 2022, 4:29 AM
Herald added a project: Restricted Project. · View Herald Transcript
nikic requested review of this revision.Jul 11 2022, 4:29 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 11 2022, 4:29 AM
reames accepted this revision.Jul 11 2022, 12:43 PM

LGTM

This revision is now accepted and ready to land.Jul 11 2022, 12:43 PM
This revision was landed with ongoing or failed builds.Jul 12 2022, 12:41 AM
This revision was automatically updated to reflect the committed changes.
jgorbe added a subscriber: jgorbe.Jul 13 2022, 3:40 PM

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)
nikic added a comment.Jul 14 2022, 8:09 AM

@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).

lerno added a subscriber: lerno.Aug 11 2022, 7:30 AM

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.