samitolvanen added a comment. In D119296#3623059 <https://reviews.llvm.org/D119296#3623059>, @nickdesaulniers wrote:
> I see you modified the mir parser & printer; consider adding a .mir test. I added a .mir test for parsing the cfi-type. The machine instructions generated for the various call types are covered in the .ll tests. > Might be nice to document the generated asm more for other compiler vendors > to better understand the implementation, rather than having to read the tests > added here. The exact instruction sequence is also documented in the kernel patches in case other compiler vendors want to implement this, but sure, that makes sense. I assume comments in the various `AsmPrinter` functions would be sufficient? ================ Comment at: clang/lib/CodeGen/CodeGenFunction.h:4604 + void EmitKCFIOperandBundle(const CGCallee &Callee, + SmallVectorImpl<llvm::OperandBundleDef> &Bundles); + ---------------- nickdesaulniers wrote: > Should this parameter be an `ArrayRef`? This might be a more precise type, > but seeing an impl type kind of breaks the whole pImpl idiom; I'm pretty sure > that's what `ArrayRef` is for. > https://llvm.org/docs/ProgrammersManual.html#llvm-adt-arrayref-h > That said, this header is pretty inconsistent in the use of `SmallVectorImp` > vs `ArrayRef`, but I *think* `ArrayRef` is strongly preferred. I don't believe I can use `ArrayRef` when I need to append to the list, it appears to be read-only. ================ Comment at: llvm/lib/CodeGen/MachineInstr.cpp:1770-1771 + if (uint32_t CFIType = getCFIType()) { + if (!FirstOp) { + FirstOp = false; + OS << ','; ---------------- nickdesaulniers wrote: > If `FirstOp` is `false`, reassign it `false`? Is that right? I see that's > what's done on L1763, but..."what?" Ah yes, it looks like this has been copied a few times earlier already. I'll stop the cycle. ================ Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:7822-7840 + ConstantInt *CFIType = nullptr; + auto Bundle = CB.getOperandBundle(LLVMContext::OB_kcfi); + if (Bundle && CB.isIndirectCall()) { + if (!TLI.supportKCFIBundles()) + report_fatal_error( + "Target doesn't support calls with kcfi operand bundles."); + CFIType = cast<ConstantInt>(Bundle->Inputs[0]); ---------------- nickdesaulniers wrote: > Are we able to reorder these? > > ``` > CLI.setDebugLoc ... > ... > if (Bundle && CB.isIndirectCall) { > ... > CLI.setCFIType ... > ``` We can reorder these, but this does follow the same convention as the rest of the function. I assume `CallLoweringInfo` was specifically designed so that the arguments can all be initialized in one statement. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119296/new/ https://reviews.llvm.org/D119296 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
