[PATCH] D72463: [Driver][X86] Add -malign-branch* and -malign-branch-within-32B-boundaries
Fangrui Song via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Fri Jan 10 19:32:07 PST 2020
MaskRay marked 5 inline comments as done.
MaskRay added inline comments.
================
Comment at: clang/include/clang/Basic/DiagnosticDriverKinds.td:254
+def err_drv_invalid_malign_branch_EQ : Error<
+ "invalid argument '%0' to -malign-branch=; must be one of: %1">;
+
----------------
skan wrote:
> skan wrote:
> > The error information "must be one of: " is kind of misleading.
> > It seems that one kind of branch can be aligned.
> The error information "must be one of: " is kind of misleading.
> It seems that "only" one kind of branch can be aligned.
Added `each element`
================
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)
----------------
skan wrote:
> MaskRay wrote:
> > skan wrote:
> > > Any reason for precluding 16?
> > I did not preclude 16.
> I am sorry that I misunderstood the word "preclude". As far as I know, we would like `AlignBranchBoundary>=32`
GNU as allows `-malign-branch-boundary=16`. I think we should allow it as well. It may not be optimal, but it does not hurt to give the user more choices.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D72463/new/
https://reviews.llvm.org/D72463
More information about the cfe-commits
mailing list