skan added a comment. In D72227 <https://reviews.llvm.org/D72227> you wrote
> I think options should follow these principles: > > 1. Different options are position independent. -mA -mB should be the same as > -mB -mA. > 2. -mA and -mno-A are position dependent and the last one wins. Sometimes, > the set may include more than 2 options, e.g. the last of -fno-pic -fpie and > -fpic wins. > 3. More specific options can override semantics of less specific options. In > our case, -malign-branch* are more specific than > -malign-branch-within-32B-boundaries. This implemetation is consistent with the principles. However, as I mentioned in D72227 <https://reviews.llvm.org/D72227>, I hold a slightly different opinion about the principles. > 1. Different options are position independent. -mA -mB should be the same as > -mB -mA. I think only when A and B are not related, (-mA -mB) == (-mB -mA). > 2. -mA and -mno-A are position dependent and the last one wins. Sometimes, > the set may include more than 2 options, e.g. the last of -fno-pic -fpie and > -fpic wins Totally agree. > 3. More specific options can override semantics of less specific options. In > our case, -malign-branch* are more specific than > -malign-branch-within-32B-boundaries. I think general options can aslo override specific options. The last one wins. e.g. The net effect of `-malign-branch=fused -malign-branch-within-32B-boundaries` should be `-malign-branch-boundary=32 -malign-branch=fused+jcc+jmp -malign-branch-prefix-size=5` ================ Comment at: clang/lib/Driver/ToolChains/Clang.cpp:2034 + << T << "fused, jcc, jmp, call, ret, indirect"; + AlignBranch = StringRef(); + } ---------------- `AlignBranch = StringRef()` seems unnecessary. ================ Comment at: clang/lib/Driver/ToolChains/Clang.cpp:2040 + if (Value.getAsInteger(10, AlignBranchBoundary) || + AlignBranchBoundary < 16 || !llvm::isPowerOf2_64(AlignBranchBoundary)) { + D.Diag(diag::err_drv_invalid_argument_to_option) ---------------- Any reason for precluding 16? ================ Comment at: clang/lib/Driver/ToolChains/Clang.cpp:2053 + << Value << A->getOption().getName(); + AlignBranch = StringRef(); + } ---------------- AlignBranch = StringRef() seems unnecessary. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72463/new/ https://reviews.llvm.org/D72463 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
