[PATCH] D142688: [Clang][Driver] Handle LoongArch multiarch tuples

Sun Apr 2 01:45:20 PDT 2023

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

================
Comment at: clang/lib/Driver/ToolChains/Arch/LoongArch.cpp:66
+  default:
+    return IsLA32 ? "ilp32d" : "lp64d";
+  }
----------------
xry111 wrote:
> SixWeining wrote:
> > Better to be `f`? (Probably most 32-bit hardwares do not support double-float? But I'm not sure about this.)
> The ISA manual says 64-bit float support is not limited by 32-bit basic ISA support (except the moving instructions accessing 64-bit GPR).
AFAIK the consensus (even inside Loongson) is that even LA32's unmarked/default ABI is going to have hard F64 available. And yes the ISA manual is consistent with this view too. (IMO LA32 should instead default to soft-float, but if Loongson's commercial roadmap can guarantee hardware FPU availability on all meaningful LA32 models then that would backfire, who knows...)

================
Comment at: clang/lib/Driver/ToolChains/Linux.cpp:41
 /// so we provide a rough mapping here.
 std::string Linux::getMultiarchTriple(const Driver &D,
                                       const llvm::Triple &TargetTriple,
----------------
SixWeining wrote:
> xen0n wrote:
> > SixWeining wrote:
> > > Missing test. Perhaps add some in `clang/test/Driver/linux-ld.c` and `clang/test/Driver/linux-header-search.cpp`? Or postpone this change until loongarch is upstreamed to Debian?
> > Thanks, I'll try adding the tests in the next revision.
> > 
> > But given [[ https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1028654 | the recent reversal of upstream dpkg support ]] (apparently due to miscommunication) it may be prudent to wait until a consensus is reached Debian-side.
> [[ https://git.dpkg.org/cgit/dpkg/dpkg.git/commit/?id=9dfffbbae | The multiarch tuples have been upstreamed to dpkg ]].  I think we can amend the change and move on now.
Sorry for the delay in processing this, I've updated the code to reflect the latest spec change. I'll still have to check the unit-test part though.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142688