[PATCH] D131716: X86: Don't fold TEST into ADD ... at GOTTPOFF/GOTNTPOFF/INDNTPOFF
James Y Knight via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Aug 12 07:02:51 PDT 2022
jyknight added inline comments.
================
Comment at: llvm/lib/Target/X86/X86InstrInfo.cpp:4085
+ // "ELF Handling for Thread-Local Storage" specifies that x86-64 GOTTPOFF, and
+ // i386 GOTNTPOFF/INDNTPOFF relocations can convert an ADD to a LEA during
----------------
MaskRay wrote:
> x86-64 psABI should be more authoritative than ""ELF Handling for Thread-Local Storage"".
OTHER references in the code point to this document. E.g. X86TargetLowering::shouldReduceLoadWidth in llvm/lib/Target/X86/X86ISelLowering.cpp:
```
// "ELF Handling for Thread-Local Storage" specifies that R_X86_64_GOTTPOFF
// relocation target a movq or addq instruction: don't let the load shrink.
```
and the definition of MO_GOTTPOFF in llvm/lib/Target/X86/MCTargetDesc/X86BaseInfo.h:
```
/// See 'ELF Handling for Thread-Local Storage' for more details.
/// SYMBOL_LABEL @GOTTPOFF
MO_GOTTPOFF,
```
I'd prefer to be consistent, so will leave this.
================
Comment at: llvm/lib/Target/X86/X86InstrInfo.cpp:4091
+ unsigned Flags = MI.getOperand(5).getTargetFlags();
+ if (Flags == X86II::MO_GOTTPOFF || Flags == X86II::MO_INDNTPOFF || Flags == X86II::MO_GOTNTPOFF)
+ return false;
----------------
MaskRay wrote:
> craig.topper wrote:
> > 80 columns
> Can use `llvm::is_contained({...}, Flags)` to make this shorter.
Turns out this is a little annoying, given that the first arg turns out to be initializer_list<X86II::TOF>, but getTargetFlags returns `unsigned int`, and llvm::is_contained is `template <typename T> constexpr bool is_contained(std::initializer_list<T> Set, T Value)`.
I could workaround with explicit casts here, but that feels uglier than just using `||` so I'll just leave this alone. (I expect that is_contained should be fixed, but I'm not doing that in this change).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D131716/new/
https://reviews.llvm.org/D131716
More information about the llvm-commits
mailing list