[PATCH] D74651: [WIP] Add intrinsics and operand bundles for inalloca replacement llvm.call.setup

[Reid Kleckner via Phabricator via llvm-commits]

rnk added a comment.

Nice, that's the essence of the verifier checks, and the tests look pretty good. LLVM has an 80 character limit, and the easiest way to handle that is to use clang-format. I recommend setting up an editor integration so you can use it interactively as you edit code, or running git-clang-format before uploading.



================
Comment at: llvm/lib/IR/Verifier.cpp:3057
+      Assert(BU.Inputs.size() == 1,
+             "Expected exactly one callsetup bundle operand", Call);
+      Assert(isa<ConstantTokenNone>(BU.Inputs.front()),
----------------
You should add some tests for these checks.


================
Comment at: llvm/lib/IR/Verifier.cpp:3061-3062
+             Call);
+    } else {
+      Assert(false, "unknown bundle");
     }
----------------
The verifier shouldn't reject unknown bundles, the idea is to allow other bundles as an extension point.


================
Comment at: llvm/lib/IR/Verifier.cpp:4441
+        Assert(AllocArgIndex != nullptr, "llvm.call.alloc arg index must be a constant");
+        Assert(AllocArgIndex->getValue().sge(0) && AllocArgIndex->getValue().slt(NumArgs->getValue()), "llvm.call.alloc arg index must be between 0 and corresponding llvm.call.setup's argument count");
+      } else {
----------------
This is the right idea, but I'd make APInt locals for the getValue() results to make it shorter.


================
Comment at: llvm/lib/IR/Verifier.cpp:4449
+        } else {
+          CheckFailed("Use of llvm.call.setup outside intrinsics must be in \"callsetup\" operand bundle");
+        }
----------------
I guess I'd try to do this with less conditionals:
  auto CallSetupBundle = ...
  Assert(CallSetupBundle, "using call site should have a call.setup bundle");
  Assert(CallSetupBundle->Inputs.front().get() == &Call, ...);



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74651