[PATCH] D72463: [Driver][X86] Add -malign-branch* and -malign-branch-within-32B-boundaries

[Kan Shengchen via Phabricator via cfe-commits]

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