Page MenuHomePhabricator

Make predefined FLT16 macros conditional on support for the type
ClosedPublic

Authored by nemanjai on Feb 1 2019, 3:25 AM.

Details

Summary

We unconditionally predefine these macros. However, they may be used to determine if the type is supported. In that case, there are unnecessary failures to compile the code.
This is the proposed fix for https://bugs.llvm.org/show_bug.cgi?id=40559

Diff Detail

Repository
rL LLVM

Event Timeline

nemanjai created this revision.Feb 1 2019, 3:25 AM
nemanjai marked an inline comment as done.Feb 1 2019, 3:26 AM
nemanjai added inline comments.
lib/Basic/Targets/WebAssembly.h
55 ↗(On Diff #184716)

There are test cases that check for the macros for WebAssembly so I assumed they probably want the type to be valid on the target. If that's not the case, I can change the test case.

sunfish added a subscriber: sunfish.Feb 1 2019, 7:00 AM

I think WebAssembly is in the same situation as most other architectures, as discussed here:

http://llvm.org/viewvc/llvm-project?view=revision&revision=352221

and should not enable _Float16 yet. The test/Preprocessor/init.c tests were semi-automatically generated, so it wasn't specifically intended to include tests for FLT16 macros for WebAssembly.

nemanjai updated this revision to Diff 185088.Feb 4 2019, 10:39 AM

As mentioned in a comment, the WASM tests weren't really meant to indicate that WASM supports the type. Removed the changes to the WASM target.

Does anyone have any further comments or objections to this patch? I would like to commit this and close the PR.

SjoerdMeijer accepted this revision.Feb 8 2019, 5:35 AM

Looks okay to me, with one nit inline.

test/Preprocessor/init.c
9169 ↗(On Diff #185088)

Perhaps change this in WEBASSEMBLY-NOT so that we also have one negative test for this?

This revision is now accepted and ready to land.Feb 8 2019, 5:35 AM
nemanjai marked an inline comment as done.Feb 20 2019, 4:36 AM

Since I haven't seen any further objections to this, I'll commit this later today.

test/Preprocessor/init.c
9169 ↗(On Diff #185088)

I will do this on the commit.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 20 2019, 12:27 PM