This is an archive of the discontinued LLVM Phabricator instance.

[clang][Interp] Implement __builtin_isnan()
ClosedPublic

Authored by tbaeder on Jul 15 2023, 6:30 AM.

Details

Summary

Add a way to pop arguments based on the call expression instead of the function and use that.

clang/test/Sema/constant-builtins-fmin.cpp is the promised test form the other reviews.

Diff Detail

Event Timeline

tbaeder created this revision.Jul 15 2023, 6:30 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 15 2023, 6:30 AM
tbaeder requested review of this revision.Jul 15 2023, 6:30 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 15 2023, 6:30 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
cor3ntin added inline comments.Jul 15 2023, 7:56 AM
clang/lib/AST/Interp/Function.h
180

You can compare getBuiltinID(), which is better than using strings, it's what we seem to be doing elsewhere

clang/lib/AST/Interp/InterpBuiltin.cpp
169

There are only a few functions created by SemaBuiltinFPClassification - only two of which are not unary. this looks fine, fpclassify and isfpclass can have their own logic.
I don't suppose there is a way to assert we have one argument there?

tbaeder added inline comments.Jul 15 2023, 8:36 AM
clang/lib/AST/Interp/Function.h
180

Yeah but that still sucks. Is there no way to query whether the builtin has t set ("signature is meaningless, use custom typechecking"), regardless of the ID or name?

clang/lib/AST/Interp/InterpBuiltin.cpp
169

We could (and maybe want) to pass along the CallExpr, but the arguments and their number are already checked in Sema.

cor3ntin added inline comments.Jul 15 2023, 8:45 AM
clang/lib/AST/Interp/Function.h
180

AstContext.BuiltinInfo.hasCustomTypechecking(F->getBuiltinID())) seems to do that but i have no clue if you could use it on all functions without running into issues

tbaeder updated this revision to Diff 540692.Jul 15 2023, 9:05 AM
tbaeder marked 2 inline comments as done.
tbaeder added inline comments.
clang/lib/AST/Interp/Function.h
180

That works indeed, thanks.

tbaeder updated this revision to Diff 540760.Jul 15 2023, 11:33 PM
tbaeder marked an inline comment as done.
cor3ntin accepted this revision.Jul 16 2023, 11:30 PM
cor3ntin added inline comments.
clang/lib/AST/Interp/InterpBuiltin.cpp
166–168

I would remove the fixme, given you already implemented all the floating point builtins

This revision is now accepted and ready to land.Jul 16 2023, 11:30 PM
tbaeder marked an inline comment as done.Jul 17 2023, 11:09 PM
tbaeder updated this revision to Diff 541362.Jul 18 2023, 12:42 AM
tbaeder marked an inline comment as done.
This revision was landed with ongoing or failed builds.Jul 28 2023, 11:50 AM
This revision was automatically updated to reflect the committed changes.