[PATCH] D73070: Add function attribute "patchable-function-prefix" to support -fpatchable-function-entry=N,M where M>0

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers marked an inline comment as done.
nickdesaulniers added a comment.

Thanks for the patch!



================
Comment at: llvm/lib/IR/Verifier.cpp:1860
+                       .getValueAsString();
+    StringRef S = S0;
+    unsigned N;
----------------
Why do we make a copy of the `StringRef`? (Sorry I didn't notice in code review of the below test case previously). I assume this has something to do with avoiding "consuming" an attribute?


================
Comment at: llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp:1303
+  while (N--)
+    EmitToStreamer(*OutStreamer, MCInstBuilder(AArch64::HINT).addImm(0));
+}
----------------
I think it would be better to use `TargetInstrInfo::getNoop()` via `MF->getSubtarget().getInstrInfo()->getNoop(Noop);`. Then you could make this a non-virtual function call in the base `AsmPrinter` class.  In fact, emitting nops is useful and could be a method on the base `AsmPrinter`, and that could be reused between `AsmPrinter::EmitFunctionHeader()` and `AsmPrinter::EmitFunctionBody()`.


================
Comment at: llvm/test/CodeGen/AArch64/patchable-function-entry-bti.ll:37
+; CHECK-NEXT:  mov w0, wzr
+; CHECK:      .size f1_1, .Lfunc_end2-f1_1
+; CHECK:      .section __patchable_function_entries,"awo", at progbits,f1,unique,0
----------------
Ditto about `.Lfunc_end2` placement (see below comment).


================
Comment at: llvm/test/CodeGen/AArch64/patchable-function-entry.ll:79
+;; .size does not include the prefix.
+; CHECK:       .size f3_2, .Lfunc_end5-f3_2
+; NOFSECT      .section __patchable_function_entries,"awo", at progbits,f1,unique,0
----------------
Can you please include where `.Lfunc_end5` should be in a `CHECK`, so that reviewers can verify that `.size` is set correctly?


================
Comment at: llvm/test/CodeGen/AArch64/patchable-function-entry.ll:80
+; CHECK:       .size f3_2, .Lfunc_end5-f3_2
+; NOFSECT      .section __patchable_function_entries,"awo", at progbits,f1,unique,0
+; FSECT:       .section __patchable_function_entries,"awo", at progbits,f3_2,unique,4
----------------
What is this `NOFSECT` checking? That there's no entry for `f1`? Why?


================
Comment at: llvm/test/CodeGen/AArch64/patchable-function-entry.ll:88
+;; When prefix data is used, arbitrarily place NOPs after prefix data.
+define void @prefix() "patchable-function-entry"="0" "patchable-function-prefix"="1" prefix i32 1 {
+; CHECK-LABEL: .type prefix, at function
----------------
TIL: https://llvm.org/docs/LangRef.html#prefix-data


================
Comment at: llvm/test/CodeGen/AArch64/patchable-function-entry.ll:91
+; CHECK-NEXT:  .word 1 // @prefix
+; CHECK:       nop
+  ret void
----------------
Please add a `CHECK` for `prefix:` after the `nop` `CHECK`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73070