[PATCH] D26412: [XRay] Support AArch64 in LLVM

[Serge Rogatch via llvm-commits]

rSerge marked 5 inline comments as done.
rSerge added inline comments.


================
Comment at: lib/Target/AArch64/AArch64AsmPrinter.cpp:155
+  //TODO: merge the logic for ELF XRay sleds at a higher level, so to avoid
+  //  code duplication as it is now for x86_64, ARM32 and AArch64.
+  if (Sleds.empty())
----------------
amehsan wrote:
> extra space in the beginning can be removed.
Leaving 1 space after `//`


================
Comment at: lib/Target/AArch64/AArch64AsmPrinter.cpp:162-170
+    if (Fn->hasComdat()) {
+      Section = OutContext.getELFSection("xray_instr_map", ELF::SHT_PROGBITS,
+        ELF::SHF_ALLOC | ELF::SHF_GROUP, 0,
+        Fn->getComdat()->getName());
+    }
+    else {
+      Section = OutContext.getELFSection("xray_instr_map", ELF::SHT_PROGBITS,
----------------
dberris wrote:
> Since these are single-statement bodies for the true and false branches, LLVM style seems to prefer not having curlies around them:
> 
> ```
> if (...)
>   Section = ...;
> else
>   Section = ...;
> ```
> 
> Also, if you do need compound statements in the branches, the `else` should be on the same line as the previous `}`. So:
> 
> ```
> if (...) {
>   ...
> } else {
>   ...
> }
> ```
Thanks for explaining. It's copy-pasted from your code =)
Changing here...


================
Comment at: lib/Target/AArch64/AArch64AsmPrinter.cpp:685-686
+    LowerPATCHABLE_FUNCTION_ENTER(*MI);
+    return;
+  case TargetOpcode::PATCHABLE_FUNCTION_EXIT:
+    LowerPATCHABLE_FUNCTION_EXIT(*MI);
----------------
dberris wrote:
> To keep it consistent with surrounding style, maybe add an empty line between the `return;` and the `case ...:`? i.e.
> 
> ```
>   case TargetOpcode::...:
>     ...
>     return;
> 
>   case TargetOpcode::...;
>     ...
>     return;
> ```
Changing.


https://reviews.llvm.org/D26412