[PATCH] D70157: Align branches within 32-Byte boundary

[Kan Shengchen via Phabricator via llvm-commits]

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