This is an archive of the discontinued LLVM Phabricator instance.

[MS] Fix double evaluation of MSVC builtin arguments
ClosedPublic

Authored by rnk on Nov 24 2020, 3:53 PM.

Details

Summary

This code got quite twisted because we consider some MSVC builtins to be
target agnostic, and some to be target specific. Target specific
intrinsics have a pattern of doing up-front argument evaluation, while
general intrinsics do not evaluate their arguments up front. As we tried
to share codepaths between the target-specific and target-agnostic
handling, we ended up doing double evaluation.

Instead, have each target handle MSVC intrinsics consistently before up
front argument evaluation. This requires passing less data around and is
more consistent with target independent intrinsic handling.

See D50979 for past examples of this bug. I noticed this while looking
into adding some more intrinsics.

Diff Detail

Event Timeline

rnk created this revision.Nov 24 2020, 3:53 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 24 2020, 3:53 PM
Herald added a subscriber: jfb. · View Herald Transcript
rnk requested review of this revision.Nov 24 2020, 3:53 PM
thakis accepted this revision.Nov 25 2020, 7:27 AM

Nice!

clang/lib/CodeGen/CGBuiltin.cpp
1019

Maybe return None here and LLVM_UNREACHABLE at the bottom?

1165

same suggestion

1311

Same suggestion

7175

This comment could answer "why" too: "...because EmitSMVCBuiltinExpr() evaluates arguments and they shouldn't be evaluated twice" (maybe worded better)

clang/lib/CodeGen/CodeGenFunction.h
4126

...where's the definition of this function? I can't see calls either. I guess this is a remnant from an earlier approach?

This revision is now accepted and ready to land.Nov 25 2020, 7:27 AM
rnk marked an inline comment as done.Nov 25 2020, 11:49 AM

Thanks!

clang/lib/CodeGen/CGBuiltin.cpp
1019

Sure, why not. Why do you like this version better?

clang/lib/CodeGen/CodeGenFunction.h
4126

Yeah, it is. I'll zap it.

This revision was automatically updated to reflect the committed changes.