Page MenuHomePhabricator

[WebAssembly] Support single-floating-point immediate value
ClosedPublic

Authored by kateinoigakukun on Apr 3 2020, 5:34 AM.

Details

Summary

As mentioned in TODO comment, casting double to float causes NaNs to change bits.
To avoid the change, this patch adds support for single-floating-point immediate value on MachineCode.

Diff Detail

Unit TestsFailed

TimeTest
2,540 msx64 debian > libarcher.races::lock-unrelated.c
Script: -- : 'RUN: at line 13'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang -fopenmp -pthread -fno-experimental-isel -g -O1 -fsanitize=thread -I /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests -I /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/src -L /mnt/disks/ssd0/agent/llvm-project/build/lib -Wl,-rpath,/mnt/disks/ssd0/agent/llvm-project/build/lib /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests/races/lock-unrelated.c -o /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/lock-unrelated.c.tmp -latomic && env TSAN_OPTIONS='ignore_noninstrumented_modules=0:ignore_noninstrumented_modules=1' /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests/deflake.bash /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/lock-unrelated.c.tmp 2>&1 | tee /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/lock-unrelated.c.tmp.log | /mnt/disks/ssd0/agent/llvm-project/build/./bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests/races/lock-unrelated.c

Event Timeline

kateinoigakukun created this revision.Apr 3 2020, 5:34 AM
kateinoigakukun edited the summary of this revision. (Show Details)

Fix for failing test cases

On some 32-bit x86 hosts, particularly those that use x87 arithmetic, loading and storing with floating-point types can change the NaN bit pattern. To preserve the bit pattern reliably, MCOperand should represent floating-point immediates by representing the bits in int32_t for 32-bit and int64_t for 64-bit. Codegen and MC don't usually look at the value, but where they do need to interpret it as a floating-point value, they can reinterpret the bits on demand.

I couldn't reproduce the failure of opt-bisect-legacy-pass-manager.ll on my x86_64 host. I can't figure out why the test suite failed.

Is looks possible that the test failure was due to an unrelated commit 210f40fe9a30212396311d265904b2d73859c53d. If so, it's possible it's fixed as of 74ab5d98d07f0b0226f45ccca8df6a450d52fb7b.

llvm/lib/Target/WebAssembly/WebAssemblyMCInstLower.cpp
294

MCOperand::createFPImm, and the other functions which also hold the value in floating-point types can also implicitly convert NaN bits. To preserve it in all cases, there should be a way to construct an MCOperand from an integer holding the bits, so this code can do something like Imm->getValueAPF()->bitcastToAPInt().getZExtValue() and pass the value in.

Similar for getFPImm on the consuming side.

@sunfish Could you take a look again?

Hi @sunfish, would you be able to have another look at this or recommend someone else for review otherwise? Thank you!

sunfish accepted this revision.May 11 2020, 3:00 PM

Looks good to me.

llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp
132

Ideally, FltOp should also store uint32_t/uint64_t instead of just a double, for the same reasons as the rest of this patch. But I think it's fine to address this later.

This revision is now accepted and ready to land.May 11 2020, 3:00 PM

Having createFPImm and createSFPImm (similarly for the add methods) seems confusing. Can we not have a uniform SFP/DFP split in the names everywhere, since you're already touching the uses of the former due to changing the types?

Having createFPImm and createSFPImm (similarly for the add methods) seems confusing. Can we not have a uniform SFP/DFP split in the names everywhere, since you're already touching the uses of the former due to changing the types?

I updated to have a uniform SFP/DFP in floating-point APIs.

foad added a subscriber: foad.Nov 11 2020, 12:26 AM

@jrtc27 @sunfish Sorry for the lack of updates for so long, I fixed the CI issue and it's ready for review ๐Ÿ™

sunfish accepted this revision.Jan 4 2021, 7:04 AM

Looks good!

kateinoigakukun added a comment.EditedJan 4 2021, 7:20 AM

@sunfish Thanks for your review! I don't have a commit access, so could you commit this patch? ๐Ÿ™

Sorry for pinging. Could you land this patch? ๐Ÿ™

This revision was automatically updated to reflect the committed changes.