This is an archive of the discontinued LLVM Phabricator instance.

Differential D13749

x86 atomic codegen: don't drop globals
ClosedPublic

Authored by jfb on Oct 14 2015, 3:23 PM.

Details

Reviewers
pete
Commits
rG5b327712b072: x86 FP atomic codegen: don't drop globals, stack
rL250429: x86 FP atomic codegen: don't drop globals, stack
Summary

x86 codegen is clever about generating good code for relaxed
floating-point operations, but it was being silly when globals and
immediates were involved, forgetting where the global was and
loading/storing from/to the wrong place.

Don't let it forget about the displacement.

This fixes https://llvm.org/bugs/show_bug.cgi?id=25171

Diff Detail

Repository
rL LLVM

Event Timeline

jfb updated this revision to Diff 37403.Oct 14 2015, 3:23 PM
jfb retitled this revision from to x86 atomic codegen: don't drop globals.
jfb updated this object.
jfb added a reviewer: pete.
jfb added subscribers: rsmith, majnemer, llvm-commits.
jfb added a comment.Oct 14 2015, 3:24 PM

The bug was introduced in D11382.

I'm not a huge fan of the code duplication, but it seems easier to read that cleverly reusing partially-constructed MIs.

Also, the code currently checks for global and immediate since they seem to be the only possible displacements. Should I test immediate? That's just a bitcast of a value, right?

Any other corner case?

majnemer added a comment.Oct 14 2015, 3:31 PM

This will crash in cases where MI->getOperand(0) is not a register.

pete edited edge metadata.Oct 14 2015, 3:34 PM
In D13749#267456, @jfb wrote:

The bug was introduced in D11382.

I'm not a huge fan of the code duplication, but it seems easier to read that cleverly reusing partially-constructed MIs.

Also, the code currently checks for global and immediate since they seem to be the only possible displacements. Should I test immediate? That's just a bitcast of a value, right?

Yeah, exactly, just a bit cast of an immediate.

Any other corner case?

Can't think of anything. I assume that a floating point literal, with a bit cast, would just constant fold to an integer anyway.

jfb added a comment.Oct 14 2015, 3:36 PM

The following bug is similar: https://llvm.org/bugs/show_bug.cgi?id=25144
Except in that one oeprand 0 is a FrameIndex, not a register.

pete added a comment.Oct 14 2015, 3:38 PM

I'm not a huge fan of the code duplication, but it seems easier to read that cleverly reusing partially-constructed MIs.

Yeah, this isn't the easiest thing to avoid here.

One option is to change addDisp to take an Optional<MachineOperand> or something like that which only applies the displacement when its set.

Another alternative is that you create a temporary MachineOperand on the stack and rely on the code in addDisp which checks for immediates, and if they are immediates just adds to them. That is, you could do

MachineOperand ZeroDisp = MachineOperand::createImm(0);
BuildMI(...)
.add...
.addDisp(hasDisp ? Disp : ZeroDisp)

then you won't have any duplication. Thats the least duplication I could come up with.

Cheers!
Pete

pete added a comment.Oct 14 2015, 3:41 PM
In D13749#267471, @jfb wrote:

The following bug is similar: https://llvm.org/bugs/show_bug.cgi?id=25144
Except in that one oeprand 0 is a FrameIndex, not a register.

Ah, yeah in that case I guess just do MIM.addOperand(MI->getOperand(0)) and it'll handle anything you give it.

jfb updated this revision to Diff 37414.Oct 14 2015, 3:53 PM
jfb edited edge metadata.
  • Check with an immediate location as well as a global.
jfb updated this revision to Diff 37422.Oct 14 2015, 4:29 PM
  • Refactor my change to be more concise as suggested by pete.
jfb updated this revision to Diff 37426.Oct 14 2015, 4:46 PM
  • Change the code a bit, it seems more correct this way.
pete accepted this revision.Oct 14 2015, 5:09 PM
pete edited edge metadata.

LGTM.

Thanks for fixing this!

This revision is now accepted and ready to land.Oct 14 2015, 5:09 PM
jfb updated this revision to Diff 37431.Oct 14 2015, 5:35 PM
jfb edited edge metadata.
  • Address the case where the base address isn't a register (see pr25144). This still needs a test, and the setIsKill is suboptimal because MIO should kill MSrc.
jfb added a comment.Oct 14 2015, 5:36 PM

Quick update before I head out (prior obligations...): I'm also addressing pr25144. There was some weirdness around kill, and it's still not perfect as-is. I also need to add the test from pr25144. I'll do that tomorrow.

jfb updated this revision to Diff 37493.Oct 15 2015, 9:43 AM
  • Also test for pr25144, and only set kill state when the src is a reg.
Closed by commit rL250429: x86 FP atomic codegen: don't drop globals, stack (authored by jfb). · Explain WhyOct 15 2015, 9:48 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents

PathSize
llvm/
trunk/
lib/
Target/
X86/
15 lines
test/
CodeGen/
X86/
108 lines

Diff 37495

llvm/trunk/lib/Target/X86/X86ISelLowering.cpp

  • This file is larger than 256 KB, so syntax highlighting is disabled by default.
Show First 20 Lines • Show All 21,099 Lines • ▼ Show 20 LinesX86TargetLowering::EmitLoweredAtomicFP(MachineInstr *MI,
switch (MI->getOpcode()) { switch (MI->getOpcode()) {
default: llvm_unreachable("unexpected instr type for EmitLoweredAtomicFP"); default: llvm_unreachable("unexpected instr type for EmitLoweredAtomicFP");
case X86::RELEASE_FADD32mr: MOp = X86::MOVSSmr; FOp = X86::ADDSSrm; break; case X86::RELEASE_FADD32mr: MOp = X86::MOVSSmr; FOp = X86::ADDSSrm; break;
case X86::RELEASE_FADD64mr: MOp = X86::MOVSDmr; FOp = X86::ADDSDrm; break; case X86::RELEASE_FADD64mr: MOp = X86::MOVSDmr; FOp = X86::ADDSDrm; break;
} }
const X86InstrInfo *TII = Subtarget->getInstrInfo(); const X86InstrInfo *TII = Subtarget->getInstrInfo();
DebugLoc DL = MI->getDebugLoc(); DebugLoc DL = MI->getDebugLoc();
MachineRegisterInfo &MRI = BB->getParent()->getRegInfo(); MachineRegisterInfo &MRI = BB->getParent()->getRegInfo();
unsigned MSrc = MI->getOperand(0).getReg(); MachineOperand MSrc = MI->getOperand(0);
unsigned VSrc = MI->getOperand(5).getReg(); unsigned VSrc = MI->getOperand(5).getReg();
const MachineOperand &Disp = MI->getOperand(3);
MachineOperand ZeroDisp = MachineOperand::CreateImm(0);
bool hasDisp = Disp.isGlobal() || Disp.isImm();
if (hasDisp && MSrc.isReg())
MSrc.setIsKill(false);
MachineInstrBuilder MIM = BuildMI(*BB, MI, DL, TII->get(MOp)) MachineInstrBuilder MIM = BuildMI(*BB, MI, DL, TII->get(MOp))
.addReg(/*Base=*/MSrc) .addOperand(/*Base=*/MSrc)
.addImm(/*Scale=*/1) .addImm(/*Scale=*/1)
.addReg(/*Index=*/0) .addReg(/*Index=*/0)
.addImm(0) .addDisp(hasDisp ? Disp : ZeroDisp, /*off=*/0)
.addReg(0); .addReg(0);
MachineInstr *MIO = BuildMI(*BB, (MachineInstr *)MIM, DL, TII->get(FOp), MachineInstr *MIO = BuildMI(*BB, (MachineInstr *)MIM, DL, TII->get(FOp),
MRI.createVirtualRegister(MRI.getRegClass(VSrc))) MRI.createVirtualRegister(MRI.getRegClass(VSrc)))
.addReg(VSrc) .addReg(VSrc)
.addReg(/*Base=*/MSrc) .addOperand(/*Base=*/MSrc)
.addImm(/*Scale=*/1) .addImm(/*Scale=*/1)
.addReg(/*Index=*/0) .addReg(/*Index=*/0)
.addImm(/*Disp=*/0) .addDisp(hasDisp ? Disp : ZeroDisp, /*off=*/0)
.addReg(/*Segment=*/0); .addReg(/*Segment=*/0);
MIM.addReg(MIO->getOperand(0).getReg(), RegState::Kill); MIM.addReg(MIO->getOperand(0).getReg(), RegState::Kill);
MI->eraseFromParent(); // The pseudo instruction is gone now. MI->eraseFromParent(); // The pseudo instruction is gone now.
return BB; return BB;
} }
MachineBasicBlock * MachineBasicBlock *
X86TargetLowering::EmitLoweredSegAlloca(MachineInstr *MI, X86TargetLowering::EmitLoweredSegAlloca(MachineInstr *MI,
▲ Show 20 Lines • Show All 6,216 Lines • Show Last 20 Lines

llvm/trunk/test/CodeGen/X86/atomic_mi.ll

Show First 20 Lines • Show All 865 Lines • ▼ Show 20 Lines; Don't check x86-32 (see comment above).
%floc = bitcast double* %loc to i64* %floc = bitcast double* %loc to i64*
%1 = load atomic i64, i64* %floc seq_cst, align 8 %1 = load atomic i64, i64* %floc seq_cst, align 8
%2 = bitcast i64 %1 to double %2 = bitcast i64 %1 to double
%add = fadd double %2, %val %add = fadd double %2, %val
%3 = bitcast double %add to i64 %3 = bitcast double %add to i64
store atomic i64 %3, i64* %floc release, align 8 store atomic i64 %3, i64* %floc release, align 8
ret void ret void
} }
@glob32 = global float 0.000000e+00, align 4
@glob64 = global double 0.000000e+00, align 8
; Floating-point add to a global using an immediate.
define void @fadd_32g() {
; X64-LABEL: fadd_32g:
; X64-NOT: lock
; X64: movss .{{[A-Z0-9_]+}}(%rip), %[[XMM:xmm[0-9]+]]
; X64-NEXT: addss glob32(%rip), %[[XMM]]
; X64-NEXT: movss %[[XMM]], glob32(%rip)
; X32-LABEL: fadd_32g:
; Don't check x86-32 (see comment above).
%i = load atomic i32, i32* bitcast (float* @glob32 to i32*) monotonic, align 4
%f = bitcast i32 %i to float
%add = fadd float %f, 1.000000e+00
%s = bitcast float %add to i32
store atomic i32 %s, i32* bitcast (float* @glob32 to i32*) monotonic, align 4
ret void
}
define void @fadd_64g() {
; X64-LABEL: fadd_64g:
; X64-NOT: lock
; X64: movsd .{{[A-Z0-9_]+}}(%rip), %[[XMM:xmm[0-9]+]]
; X64-NEXT: addsd glob64(%rip), %[[XMM]]
; X64-NEXT: movsd %[[XMM]], glob64(%rip)
; X32-LABEL: fadd_64g:
; Don't check x86-32 (see comment above).
%i = load atomic i64, i64* bitcast (double* @glob64 to i64*) monotonic, align 8
%f = bitcast i64 %i to double
%add = fadd double %f, 1.000000e+00
%s = bitcast double %add to i64
store atomic i64 %s, i64* bitcast (double* @glob64 to i64*) monotonic, align 8
ret void
}
; Floating-point add to a hard-coded immediate location using an immediate.
define void @fadd_32imm() {
; X64-LABEL: fadd_32imm:
; X64-NOT: lock
; X64: movl $3735928559, %e[[M:[a-z]+]]
; X64: movss .{{[A-Z0-9_]+}}(%rip), %[[XMM:xmm[0-9]+]]
; X64-NEXT: addss (%r[[M]]), %[[XMM]]
; X64-NEXT: movss %[[XMM]], (%r[[M]])
; X32-LABEL: fadd_32imm:
; Don't check x86-32 (see comment above).
%i = load atomic i32, i32* inttoptr (i32 3735928559 to i32*) monotonic, align 4
%f = bitcast i32 %i to float
%add = fadd float %f, 1.000000e+00
%s = bitcast float %add to i32
store atomic i32 %s, i32* inttoptr (i32 3735928559 to i32*) monotonic, align 4
ret void
}
define void @fadd_64imm() {
; X64-LABEL: fadd_64imm:
; X64-NOT: lock
; X64: movl $3735928559, %e[[M:[a-z]+]]
; X64: movsd .{{[A-Z0-9_]+}}(%rip), %[[XMM:xmm[0-9]+]]
; X64-NEXT: addsd (%r[[M]]), %[[XMM]]
; X64-NEXT: movsd %[[XMM]], (%r[[M]])
; X32-LABEL: fadd_64imm:
; Don't check x86-32 (see comment above).
%i = load atomic i64, i64* inttoptr (i64 3735928559 to i64*) monotonic, align 8
%f = bitcast i64 %i to double
%add = fadd double %f, 1.000000e+00
%s = bitcast double %add to i64
store atomic i64 %s, i64* inttoptr (i64 3735928559 to i64*) monotonic, align 8
ret void
}
; Floating-point add to a stack location.
define void @fadd_32stack() {
; X64-LABEL: fadd_32stack:
; X64-NOT: lock
; X64: movss .{{[A-Z0-9_]+}}(%rip), %[[XMM:xmm[0-9]+]]
; X64-NEXT: addss [[STACKOFF:-?[0-9]+]](%rsp), %[[XMM]]
; X64-NEXT: movss %[[XMM]], [[STACKOFF]](%rsp)
; X32-LABEL: fadd_32stack:
; Don't check x86-32 (see comment above).
%ptr = alloca i32, align 4
%bc3 = bitcast i32* %ptr to float*
%load = load atomic i32, i32* %ptr acquire, align 4
%bc0 = bitcast i32 %load to float
%fadd = fadd float 1.000000e+00, %bc0
%bc1 = bitcast float %fadd to i32
store atomic i32 %bc1, i32* %ptr release, align 4
ret void
}
define void @fadd_64stack() {
; X64-LABEL: fadd_64stack:
; X64-NOT: lock
; X64: movsd .{{[A-Z0-9_]+}}(%rip), %[[XMM:xmm[0-9]+]]
; X64-NEXT: addsd [[STACKOFF:-?[0-9]+]](%rsp), %[[XMM]]
; X64-NEXT: movsd %[[XMM]], [[STACKOFF]](%rsp)
; X32-LABEL: fadd_64stack:
; Don't check x86-32 (see comment above).
%ptr = alloca i64, align 8
%bc3 = bitcast i64* %ptr to double*
%load = load atomic i64, i64* %ptr acquire, align 8
%bc0 = bitcast i64 %load to double
%fadd = fadd double 1.000000e+00, %bc0
%bc1 = bitcast double %fadd to i64
store atomic i64 %bc1, i64* %ptr release, align 8
ret void
}