[PATCH] D158587: [sanitizer][msan] VarArgHelper for loongarch64

[Vitaly Buka via Phabricator via llvm-commits]

vitalybuka added inline comments.


================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:5840
+struct VarArgLoongArch64Helper : public VarArgHelper {
+  Function &F;
+  MemorySanitizer &MS;
----------------
Ami-zhang wrote:
> vitalybuka wrote:
> > It looks like exact clone of Mips64 helper.
> > Only `      if (TargetTriple.getArch() == Triple::mips64) {` is missing
> > 
> > Why not just use Mips64 then?
> Yes, the `VarArg` convention  in LoongArch's ABI is similar to that of Mips,  so `VarArgLoongArch64Helper` is here based on Mips.
> 
> If use Mips64 helper, like this:
> 
> ```
> +  else if (TargetTriple.isMIPS64() || TargetTriple.isLoongArch64())
>      return new VarArgMIPS64Helper(Func, Msan, Visitor);
> ```
> `VarArgMIPS64Helper` is a MIPS64-specific implementation. Adding `loongarch64` here, does it seem somewhat ambiguous, suggesting `LoongArch` as a variant of `MIPS `architecture? I don't know if it's good to use it directly like mentioned above. So is it okay like above or have any better suggestions?
> 
> Thanks for your time.
> 
How about something line this:

// Loongarch64 is not a MIPS, but the current vargs calling convention matches the MIPS.
using VarArgLoongArch64Helper = VarArgMIPS64Helper;

Another alternative is to rename VarArgMIPS64Helper into something more general, maybe there is a name for this calling convention already?


================
Comment at: llvm/test/Instrumentation/MemorySanitizer/LoongArch/vararg-loongarch64.ll:3
+
+target datalayout = "e-m:e-p:64:64-i64:64-i128:128-n64-S128"
+target triple = "loongarch64-unknown-linux-gnu"
----------------
even if we use MIPS helper, please keep this new test, in case someone modify Mips part ignoring loongarch64


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158587