[PATCH] D39820: [cfi-verify] Validate there are no spills between CFI-check and instruction execution.

[Mitch Phillips via Phabricator via llvm-commits]

hctim added inline comments.


================
Comment at: tools/llvm-cfi-verify/lib/FileAnalysis.cpp:294
+  // Now check all branches to indirect CFs and ensure no spill happens.
+  for (const auto Branch : Graph.ConditionalBranchNodes) {
+    uint64_t Node;
----------------
vlad.tsyrklevich wrote:
> I think Branch should explicitly be declared a reference to avoid copies.
Thanks, nice catch!


================
Comment at: tools/llvm-cfi-verify/lib/FileAnalysis.cpp:306
+      for (unsigned RegNum : RegisterNumbers) {
+        if (InstrDesc.hasImplicitDefOfPhysReg(RegNum, RegisterInfo.get()))
+          return Node;
----------------
vlad.tsyrklevich wrote:
> Reading the MCInstrDesc docs it makes it seem like this will ONLY check for implicit uses, not explicit uses. It's confusing why then  your `add $0, %eax` unit tests below works, maybe it' s because you're implicitly defining rax by adding to eax. Could you try using an add rax instruction instead and see if that works?
I was similarly confused by the naming/comment of `hasImplicitDefOfPhysReg`, I've added a test and it's all good. It's interesting as `getImplicitUses` is described as "Return a list of registers that are potentially read by any instance of this machine instruction", which would indicate that it also gets the explicit uses.

I'm not sure why the documentation is (apparently) wrong for `hasImplicitDefOfPhysReg`...


https://reviews.llvm.org/D39820