rengolin added a comment. This looks great, thanks!
Exciting that we can finally go all the way from C source to LoongArch binary. The changes look good to me, other than a few nits. But please wait for a day or two for other people to review on their own time. ================ Comment at: clang/lib/Basic/Targets/LoongArch.h:69 + // TODO: select appropriate ABI. + ABI = "ilp32d"; + } ---------------- nit: this should use `setABI`. Right now, there's no difference, but once the function does more (like setting other ABI flags), you won't need to change it here. Same for the 64-bit version. ================ Comment at: clang/lib/Driver/ToolChains/Arch/LoongArch.h:25 +} // end namespace loongarch +} // namespace tools +} // end namespace driver ---------------- nit: comment mismatch "end" ================ Comment at: clang/test/Driver/loongarch-abi.c:41 + +// CHECK-LA32-LP64S: error: unknown target ABI 'lp64s' +// CHECK-LA32-LP64F: error: unknown target ABI 'lp64f' ---------------- please, split between pass and error by adding a new `loongarch-abi-error.c` and adding the `error` tests to it. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130255/new/ https://reviews.llvm.org/D130255 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
