[PATCH] D131716: X86: Don't fold TEST into ADD ... at GOTTPOFF/GOTNTPOFF/INDNTPOFF

[James Y Knight via Phabricator via llvm-commits]

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