ychen added a comment. In D119301#3309253 <https://reviews.llvm.org/D119301#3309253>, @ychen wrote:
> In D119301#3309140 <https://reviews.llvm.org/D119301#3309140>, @lenary wrote: > >> I don't fully understand the reasoning for the patch, and you haven't really >> explained it. I think what you are saying is that the `IsAux` argument to >> `getTargetFeatures` should be considered because it's `true` for offloading >> to another compiler, but I don't understand why we think the offload >> compiler is not clang-compatible, as the features added in >> `getAArch64TargetFeatures` and `getARMTargetFeatures` (and passed to the >> offloaded compiler) use the LLVM-specific internal names. I would like you >> to explain this further, and to document what `IsAux` implies in the code. > > Apologies for the lack of explanation. My point is that > `getAArch64TargetFeatures` / `getARMTargetFeatures` should only get the > TargetFeatures, not adding compiler flags as a side effect, which belongs to > clang's job construction. This makes code reuse harder. > >> That all said code given here is not equivalent to the feature as originally >> landed, because you've only re-added the `-Wunaligned-access` flag for the >> AArch64 target - please make the same addition in `Clang::AddARMTargetArgs` >> as we want the warning on both. I think this also makes `CmdArgs` unused in >> both `aarch64::getAArch64TargetFeatures` and `arm::getARMTargetFeatures` as >> I think that you don't actually want us adding to `CmdArgs` in those >> functions (again, I hope the full reasoning behind this will be explained >> when you explain why the whole patch is necessary). > > Will do the same thing for `Clang::AddARMTargetArgs`. Thanks. I've tested > this patch but not sure why the original test still passing without a > `Clang::AddARMTargetArgs` change. NVM. I see why: there were no driver tests. I'll add those. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119301/new/ https://reviews.llvm.org/D119301 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
