SixWeining added a comment. In D130255#3668436 <https://reviews.llvm.org/D130255#3668436>, @rengolin wrote:
> 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. Thanks, Renato. Because relocation hasn't been added to the backend, complex code (e.g. with function call) can't been compiled currently. But at least, as you said, we finally go all the way from C source to LoongArch binary. This is a start and further implementation will be added later. ================ Comment at: clang/lib/Basic/Targets/LoongArch.h:69 + // TODO: select appropriate ABI. + ABI = "ilp32d"; + } ---------------- rengolin wrote: > 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. Thanks. No problem. ================ Comment at: clang/lib/Driver/ToolChains/Arch/LoongArch.h:25 +} // end namespace loongarch +} // namespace tools +} // end namespace driver ---------------- rengolin wrote: > nit: comment mismatch "end" Thanks. ================ 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' ---------------- rengolin wrote: > please, split between pass and error by adding a new `loongarch-abi-error.c` > and adding the `error` tests to it. Thanks. No problem. 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
