[PATCH] D142685: [LoongArch] Implement handling of triple-implied ABIs

[WÁNG Xuěruì via Phabricator via llvm-commits]

xen0n added inline comments.


================
Comment at: llvm/lib/Target/LoongArch/MCTargetDesc/LoongArchBaseInfo.cpp:24
 
+ABI computeTargetABI(const Triple &TT, StringRef ABIName) {
+  ABI ArgProvidedABI = getTargetABI(ABIName);
----------------
SixWeining wrote:
> Shall we handle `FeatureBits` as well?
I saw RISCV include `FeatureBits` in their `computeTargetABI`, but right now we're not making use of it (we don't base our selection of bitness on the parsed `FeatureBits`). I'd say let's add it later when needed.


================
Comment at: llvm/lib/Target/LoongArch/MCTargetDesc/LoongArchBaseInfo.cpp:50-52
+      errs() << "'" << ABIName
+             << "' is not a recognized ABI for this target, ignoring and using "
+                "triple-implied ABI\n";
----------------
SixWeining wrote:
> Missing test?
Thanks for checking, I'll cover this branch later.


================
Comment at: llvm/test/CodeGen/LoongArch/target-abi-from-triple.ll:1
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: not --crash llc --mtriple=loongarch32-linux-gnusf < %s 2>&1 \
----------------
SixWeining wrote:
> Is this test autogenerated?
The generated code is, but the other isn't. Given you already hesitated on this matter, let me adjust then.


================
Comment at: llvm/test/CodeGen/LoongArch/target-abi-from-triple.ll:2
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: not --crash llc --mtriple=loongarch32-linux-gnusf < %s 2>&1 \
+; RUN:   | FileCheck %s --check-prefix=ILP32S
----------------
SixWeining wrote:
> I'm not sure whether we should add tests causing crash that may slow down testing. I remeber @MaskRay once suggested me not to do so.
I vaguely remember such an occasion too, let me remove the crash assertions and replace with simple comments then. It's not like I'm not coming back to remove the TODOs ;-)


================
Comment at: llvm/test/CodeGen/LoongArch/target-abi-from-triple.ll:80-84
+;; NOTE: These prefixes are unused and the list is autogenerated. Do not add tests below this line:
+; 32ON64: {{.*}}
+; 64ON32: {{.*}}
+; CONFLICT-ILP32D: {{.*}}
+; CONFLICT-LP64D: {{.*}}
----------------
SixWeining wrote:
> Are these lines useful?
They're added automatically by the `update_llc_test_checks.py` script. I'll remove them.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142685