samitolvanen added a comment. > Needs some thoughts to merge the above paragraphs with the existing one
Great points. I updated the commit message and tried to capture most of these, PTAL. ================ Comment at: llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp:332 + continue; + while (!isRegAvailable(NextReg)) + ++NextReg; ---------------- MaskRay wrote: > As a minor optimization to skip one redundant `isRegAvailable` call: `while > (!isRegAvailable(++NextReg));` `Reg` != `NextReg` here, and using `while (!isRegAvailable(++NextReg))` would always skip the first alternative register by first incrementing `NextReg` before calling `isRegsAvailable`. What am I missing? ================ Comment at: llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp:368 + // Load the expected type hash. + const int64_t Type = MI.getOperand(1).getImm(); + const int64_t Hi20 = ((Type + 0x800) >> 12) & 0xFFFFF; ---------------- MaskRay wrote: > `int64_t` is the return type of `getImm`, not the original `KCFI_CHECK` > operand type. Casting this to a `uint32_t` is perhaps clearer. I don't think casting this to `uint32_t` make sense since we'd have to cast it back to a larger type immediately below (to prevent `+ 0x800` from overflowing). I changed the comment to indicate this is a 32-bit value though. ================ Comment at: llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp:375 + MCInstBuilder(RISCV::LUI).addReg(ScratchRegs[1]).addImm(Hi20)); + if (Lo12 || Hi20 == 0) + EmitToStreamer(*OutStreamer, ---------------- MaskRay wrote: > Is `Hi20 == 0` tested? Good catch, added a test. ================ Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:14427 SDValue Callee = CLI.Callee; + bool IsCFICall = CLI.CB && CLI.CB->isIndirectCall() && CLI.CFIType; bool &IsTailCall = CLI.IsTailCall; ---------------- MaskRay wrote: > If `CLI.CFIType != 0`, is `CLI.CB->isIndirectCall()` possible? If not, add an > assert? > This is an opportunity to save a redundant check. Sure. `CFIType` should be set only for indirect calls when we reach this point. ================ Comment at: llvm/test/CodeGen/RISCV/kcfi-patchable-function-prefix.ll:4 + +; NOC: .p2align 2 +; C: .p2align 1 ---------------- MaskRay wrote: > f1/f2/f3/f4 have the same shape. Without increasing the number of functions, > we can make some disturbance (e.g. add one extra indirect call, use inline > asm to clobber some registers) to check various cases without using .mir > tests. `.mir` tests are fine, but the signal-to-noise ratio is not great and > readers can be lost in the paramemters... There are four functions because we're testing type hash locations in relation to patchable-function-prefix nops in four different cases, in addition to the code generated for KCFI checks. I'm not sure there's a way to do this with fewer functions. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D148385/new/ https://reviews.llvm.org/D148385 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
