[PATCH] D92061: [MS] Fix double evaluation of MSVC builtin arguments

[Nico Weber via Phabricator via cfe-commits]

thakis accepted this revision.
thakis added a comment.
This revision is now accepted and ready to land.

Nice!



================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:1019
+  default:
+    break;
+  case ARM::BI_BitScanForward:
----------------
Maybe `return None` here and LLVM_UNREACHABLE at the bottom?


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:1165
+  default:
+    break;
+  case AArch64::BI_BitScanForward:
----------------
same suggestion


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:1311
+  default:
+    break;
+  case clang::X86::BI_BitScanForward:
----------------
Same suggestion


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:7175
 
+  // Handle MSVC intrinsics before argument evaluation.
+  if (Optional<MSVCIntrin> MsvcIntId = translateArmToMsvcIntrin(BuiltinID))
----------------
This comment could answer "why" too: "...because EmitSMVCBuiltinExpr() evaluates arguments and they shouldn't be evaluated twice" (maybe worded better)


================
Comment at: clang/lib/CodeGen/CodeGenFunction.h:4126
+  llvm::Value *EmitEvaluatedMSVCBuiltin(MSVCIntrin BuiltinID, const CallExpr *E,
+                                        ArrayRef<llvm::Value *> Ops);
 
----------------
...where's the definition of this function? I can't see calls either. I guess this is a remnant from an earlier approach?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D92061/new/

https://reviews.llvm.org/D92061