This is an archive of the discontinued LLVM Phabricator instance.

Differential D73606

[X86] matchAdd: don't fold a large offset into a %rip relative address
ClosedPublic

Authored by MaskRay on Jan 28 2020, 10:13 PM.

Details

Reviewers
None
Commits
rGbc15bf66dcca: [X86] matchAdd: don't fold a large offset into a %rip relative address
Summary

For ret i64 add (i64 ptrtoint (i32* @foo to i64), i64 1701208431),

X86DAGToDAGISel::matchAdd
  ...
// AM.setBaseReg(CurDAG->getRegister(X86::RIP, MVT::i64));
  if (!matchAddressRecursively(N.getOperand(0), AM, Depth+1) &&
// Try folding offset but fail; there is a symbolic displacement, so offset cannot be too large
      !matchAddressRecursively(Handle.getValue().getOperand(1), AM, Depth+1))
    return false;
  ...
  // Try again after commuting the operands.
// AM.Disp = Val; foldOffsetIntoAddress() does not know there will be a symbolic displacement
  if (!matchAddressRecursively(Handle.getValue().getOperand(1), AM, Depth+1) &&
// AM.setBaseReg(CurDAG->getRegister(X86::RIP, MVT::i64));
      !matchAddressRecursively(Handle.getValue().getOperand(0), AM, Depth+1))
// Succeeded! Produced leaq sym+disp(%rip),...
    return false;

foldOffsetIntoAddress() currently does not know there is a symbolic
displacement and can fold a large offset.

The produced leaq sym+disp(%rip), %rax instruction is relocated by
an R_X86_64_PC32. If disp is large and sym+disp-rip>=2**31, there
will be a relocation overflow.

Diff Detail

Event Timeline

MaskRay created this revision.Jan 28 2020, 10:13 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 28 2020, 10:13 PM
Herald added subscribers: llvm-commits, hiraditya. · View Herald Transcript
merge_guards_bot added a subscriber: merge_guards_bot.Jan 28 2020, 10:24 PM

Unit tests: pass. 62283 tests passed, 0 failed and 827 were skipped.

clang-tidy: pass.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

MaskRay updated this revision to Diff 241052.Jan 28 2020, 10:24 PM

Update description

Harbormaster failed remote builds in B45202: Diff 241051!Jan 28 2020, 10:25 PM
MaskRay updated this revision to Diff 241053.Jan 28 2020, 10:25 PM

Rename

MaskRay updated this revision to Diff 241054.Jan 28 2020, 10:28 PM
MaskRay retitled this revision from [X86] matchAdd: don't fold large displacements to [X86] matchAdd: don't fold a large offset into a %rip relative address.
MaskRay edited the summary of this revision. (Show Details)

Update title

This revision was not accepted when it landed; it landed in state Needs Review.Jan 28 2020, 10:34 PM
Closed by commit rGbc15bf66dcca: [X86] matchAdd: don't fold a large offset into a %rip relative address (authored by MaskRay). · Explain Why
This revision was automatically updated to reflect the committed changes.
merge_guards_bot added a comment.Jan 28 2020, 10:36 PM

Unit tests: pass. 62283 tests passed, 0 failed and 827 were skipped.

clang-tidy: pass.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

merge_guards_bot added a comment.Jan 28 2020, 10:39 PM

Unit tests: pass. 62283 tests passed, 0 failed and 827 were skipped.

clang-tidy: pass.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

Harbormaster failed remote builds in B45203: Diff 241052!Jan 28 2020, 10:43 PM
Harbormaster failed remote builds in B45204: Diff 241053!
merge_guards_bot added a comment.Jan 28 2020, 10:46 PM

Unit tests: pass. 62283 tests passed, 0 failed and 827 were skipped.

clang-tidy: pass.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

Harbormaster failed remote builds in B45205: Diff 241054!Jan 28 2020, 10:52 PM
MaskRay added a comment.Jan 28 2020, 11:33 PM

absl/hash/internal/hash.h takes the address of a static variable absl::hash_internal::CityHashState::kSeed:

ABSL_ATTRIBUTE_ALWAYS_INLINE static uint64_t Seed() {
  return static_cast<uint64_t>(reinterpret_cast<uintptr_t>(kSeed));
}
static const void* const kSeed;

With certain LTO optimizations, the address can be added to a large constant 1701208431. It is similar to ret i64 add (i64 ptrtoint (i32* @foo to i64), i64 1701208431).
Before this patch, we could produce an instruction like leaq kSeed+1701208431(%rip), %rax. At the linking stage, it is an R_X86_64_PC32 relocation overflow if kSeed-rip+1701208431 >= 0x80000000. This means kSeed-rip cannot be larger than 0x80000000-1701208431=0x1a999e91. This upper bound is not large, and some large programs can exceed the bound.

MaskRay mentioned this in D73601: [X86] Fix isOffsetSuitableForCodeModel.Jan 28 2020, 11:34 PM
MaskRay mentioned this in D73518: [ELF] Mention symbol name in reportRangeError().Jan 28 2020, 11:49 PM
craig.topper mentioned this in D73608: [X86] Alternative implementation of D73606 matchAdd: don't fold a large offset into a %rip relative address.Jan 29 2020, 12:44 AM
craig.topper mentioned this in rG1ef8e8b41429: [X86] Don't exit from foldOffsetIntoAddress if the Offset is 0, but AM.Disp is….Jan 29 2020, 9:37 PM
craig.topper mentioned this in rGd975910c50fb: [X86] Don't exit from foldOffsetIntoAddress if the Offset is 0, but AM.Disp is….Feb 1 2020, 11:38 AM

Revision Contents

PathSize
llvm/
lib/
Target/
X86/
22 lines
test/
CodeGen/
X86/
41 lines

Diff 241056

llvm/lib/Target/X86/X86ISelDAGToDAG.cpp

Show First 20 Lines • Show All 1,575 Lines • ▼ Show 20 Linesbool X86DAGToDAGISel::matchAdd(SDValue &N, X86ISelAddressMode &AM,
// Add an artificial use to this node so that we can keep track of // Add an artificial use to this node so that we can keep track of
// it if it gets CSE'd with a different node. // it if it gets CSE'd with a different node.
HandleSDNode Handle(N); HandleSDNode Handle(N);
X86ISelAddressMode Backup = AM; X86ISelAddressMode Backup = AM;
if (!matchAddressRecursively(N.getOperand(0), AM, Depth+1) && if (!matchAddressRecursively(N.getOperand(0), AM, Depth+1) &&
!matchAddressRecursively(Handle.getValue().getOperand(1), AM, Depth+1)) !matchAddressRecursively(Handle.getValue().getOperand(1), AM, Depth+1))
return false; return false;
AM = Backup;
// Try again after commuting the operands. // Don't try commuting operands if the address is in the form of
// sym+disp(%rip). foldOffsetIntoAddress() currently does not know there is a
// symbolic displacement and would fold disp. If disp is just a bit smaller
// than 2**31, it can easily cause a relocation overflow.
bool NoCommutate = false;
if (AM.isRIPRelative() && AM.hasSymbolicDisplacement())
if (ConstantSDNode *Cst =
dyn_cast<ConstantSDNode>(Handle.getValue().getOperand(1)))
NoCommutate = Cst->getSExtValue() != 0;
AM = Backup;
if (!NoCommutate) {
// Try again after commutating the operands.
if (!matchAddressRecursively(Handle.getValue().getOperand(1), AM, Depth+1) && if (!matchAddressRecursively(Handle.getValue().getOperand(1), AM, Depth + 1) &&
!matchAddressRecursively(Handle.getValue().getOperand(0), AM, Depth+1)) !matchAddressRecursively(Handle.getValue().getOperand(0), AM, Depth + 1))
return false; return false;
}
AM = Backup; AM = Backup;
// If we couldn't fold both operands into the address at the same time, // If we couldn't fold both operands into the address at the same time,
// see if we can just put each operand into a register and fold at least // see if we can just put each operand into a register and fold at least
// the add. // the add.
if (AM.BaseType == X86ISelAddressMode::RegBase && if (AM.BaseType == X86ISelAddressMode::RegBase &&
!AM.Base_Reg.getNode() && !AM.Base_Reg.getNode() &&
!AM.IndexReg.getNode()) { !AM.IndexReg.getNode()) {
▲ Show 20 Lines • Show All 3,707 Lines • Show Last 20 Lines

llvm/test/CodeGen/X86/fold-add-pcrel.ll

  • This file was added.
; RUN: llc -mtriple=x86_64 -relocation-model=static < %s | FileCheck --check-prefixes=CHECK,STATIC %s
; RUN: llc -mtriple=x86_64 -relocation-model=pic < %s | FileCheck --check-prefixes=CHECK,PIC %s
; RUN: llc -mtriple=x86_64 -code-model=medium -relocation-model=static < %s | FileCheck --check-prefixes=CHECK,MSTATIC %s
; RUN: llc -mtriple=x86_64 -code-model=medium -relocation-model=pic < %s | FileCheck --check-prefixes=CHECK,MPIC %s
@foo = dso_local global i32 0
define dso_local i64 @zero() {
; CHECK-LABEL: zero:
; CHECK: # %bb.0:
; STATIC-NEXT: movl $foo, %eax
; STATIC-NEXT: retq
; PIC-NEXT: leaq foo(%rip), %rax
; PIC-NEXT: retq
; MSTATIC-NEXT: movabsq $foo, %rax
; MSTATIC-NEXT: retq
; MPIC-NEXT: leaq _GLOBAL_OFFSET_TABLE_(%rip), %rcx
; MPIC-NEXT: movabsq $foo@GOTOFF, %rax
; MPIC-NEXT: addq %rcx, %rax
entry:
ret i64 add (i64 ptrtoint (i32* @foo to i64), i64 0)
}
;; Check we don't fold a large offset into leaq, otherwise
;; the large r_addend can easily cause a relocation overflow.
define dso_local i64 @large() {
; CHECK-LABEL: large:
; CHECK: # %bb.0:
; STATIC-NEXT: movl $1701208431, %eax
; STATIC-NEXT: leaq foo(%rax), %rax
; PIC-NEXT: leaq foo(%rip), %rax
; PIC-NEXT: addq $1701208431, %rax
; MSTATIC-NEXT: movabsq $foo, %rax
; MSTATIC-NEXT: addq $1701208431, %rax
; MSTATIC-NEXT: retq
; MPIC-NEXT: leaq _GLOBAL_OFFSET_TABLE_(%rip), %rax
; MPIC-NEXT: movabsq $foo@GOTOFF, %rcx
; MPIC-NEXT: leaq 1701208431(%rax,%rcx), %rax
entry:
ret i64 add (i64 ptrtoint (i32* @foo to i64), i64 1701208431)
}