skan added a comment. In D70157#1746793 <https://reviews.llvm.org/D70157#1746793>, @MaskRay wrote:
> On x86, the preferred function alignment is 16 > (https://github.com/llvm/llvm-project/blob/arcpatch-D70157/llvm/lib/Target/X86/X86ISelLowering.cpp#L1893), > which is the default function alignment in text sections. If the > cross-boundary decision is made with alignment=32 > (--x86-align-branch-boundary=32) in mind, and the section alignment is still > 16 (not increased to 32 or higher), the linker may place the section at an > address which equals 16 modulo 32, the section contents will thus shift by > 16. The instructions that do not cross the boundary in the object files may > cross the boundary in the linker output. Have you considered increasing the > section alignment to 32? > > Shall we default to -mbranches-within-32B-boundaries if the specified -march= > or -mtune= may be affected by the erratum? Yes, I have considered increasing the section alignment to 32 if any branch need to be aligned in this section. You can find related code at the end of fuction `alignBranchesEnd`. I think the users should have the right not to align branches even if their arch may be affected by the erratum, so currently I don't do that. ================ Comment at: llvm/lib/MC/MCFragment.cpp:435 + OS << "\n "; + OS << "Subtype:" << MF->SubKind << "Size: " << MF->getSize(); + break; ---------------- MaskRay wrote: > ``` > error: use of overloaded operator '<<' is ambiguous (with opera > nd types 'llvm::raw_ostream' and 'llvm::MCMachineDependentFragment::SubType') > ``` > > It seems you haven't defined `raw_ostream &operator<<(raw_ostream &OS, > MCMachineDependentFragment::SubType)`. I believe you also missed a space > before `Size: `. `llvm::MCMachineDependentFragment::SubType` is an enum type, it seems okay for me to not define `raw_ostream &operator<<(raw_ostream &OS, MCMachineDependentFragment::SubType)` and I could not reproduce the error. However, it's not clear to print a enum in the dump information. I change the code to print a string. ================ Comment at: llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp:88 + SmallVector<StringRef, 6> BranchTypes; + StringRef(Val).split(BranchTypes, '-', -1, false); + for (auto BranchType : BranchTypes) { ---------------- MaskRay wrote: > skan wrote: > > craig.topper wrote: > > > skan wrote: > > > > craig.topper wrote: > > > > > skan wrote: > > > > > > chandlerc wrote: > > > > > > > I feel like a comma-separated list would be a bit more clear... > > > > > > We can't use comma-separated list because we need pass the option > > > > > > with flto. > > > > > > "-Wl,-plugin-opt=--x86-align-branch-boundary=32,--plugin-opt=-x86-align-branch=fused,jcc,jmp,--plugin-opt=-x86-align-branch-prefix-size=5" > > > > > > would cause a compile fail because "jcc" was recognized as another > > > > > > option rather than a part of option > > > > > > "-x86-align-branch=fused,jcc,jmp" > > > > > Isn't there some way to nest quotes into the part of after > > > > > -plugin-opt= ? > > > > I tried to use --plugin-opt=-x86-align-branch="fused,jcc,jmp", it > > > > didn't work. > > > What if you put —plugin-opt=“-x86-align-branch=fused,jcc,jmp” > > It doesn't work too. > Both gcc and clang just split the -Wl argument by comma. There is no escape > character. For reference, > https://sourceware.org/ml/binutils/2019-11/msg00173.html is the GNU as patch > on the binutils side. > > Have you considered `+` plus? I think it may be more intuitive. Yes, I agree with you. Done ================ Comment at: llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp:173 + + bool needAlignBranch() const; + bool needAlignJcc() const; ---------------- MaskRay wrote: > We can generalize these functions with a function that takes a parameter. We already have a generalized function `bool needAlign(const MCInst &Inst) const`. `needAlignJcc()` is only a helper function that makes code more readable. ================ Comment at: llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp:411 + cast<MCSymbolRefExpr>(*Operand.getExpr()).getSymbol().getName(); + if (SymbolName.contains("__tls_get_addr")) + return true; ---------------- MaskRay wrote: > Check 64bit, then use exact comparison with either `___tls_get_addr` or > `__tls_get_addr` Why? There exists TLS Call in 32bit mode. Does `SymbolName.contains("__tls_get_addr")` possibly include more calls rather TLS Call? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70157/new/ https://reviews.llvm.org/D70157 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
