This is an archive of the discontinued LLVM Phabricator instance.

x86 atomic codegen: don't drop globals
ClosedPublic

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

Details

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?

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.
This revision was automatically updated to reflect the committed changes.