This is an archive of the discontinued LLVM Phabricator instance.

[X86] Fix a bug with fetch_add(INT32_MIN)
ClosedPublic

Authored by morisset on Oct 7 2014, 3:03 PM.

Details

Summary

Fix pr21099

The pseudocode of what we were doing (spread through two functions) was:

if (operand.doesNotFitIn32Bits())
  Opc.initializeWithFoo();
if (operand < 0)
  operand = -operand;
if (operand.doesFitIn8Bits())
  Opc.initializeWithBar();
else if (operand.doesFitIn32Bits())
  Opc.initializeWithBlah();
doStuff(Opc);

So for operand == INT32_MIN, Opc was never initialized because the operand changes
from fitting in 32 bits to not fitting, causing the various bugs/error messages
noted by pr21099.

This patch adds an extra test at the beginning for this case, and an
llvm_unreachable to have better error message if the operand ends up
not fitting in 32-bits at the end.

Diff Detail

Event Timeline

morisset updated this revision to Diff 14531.Oct 7 2014, 3:03 PM
morisset retitled this revision from to [X86] Fix a bug with fetch_add(INT32_MIN).
morisset updated this object.
morisset edited the test plan for this revision. (Show Details)
morisset added a reviewer: jfb.
morisset added a subscriber: Unknown Object (MLST).
morisset updated this object.Oct 7 2014, 3:04 PM
morisset updated this revision to Diff 14533.Oct 7 2014, 3:14 PM

Based on an offline comment by jf, improve the patch so that the immediate is
merged into the addq.

jfb added inline comments.Oct 7 2014, 4:14 PM
lib/Target/X86/X86ISelDAGToDAG.cpp
1717

The key thing to understand is that negating INT32_MIN makes it not fit in 32-bits. I'm not sure the comment makes that clear.

1718

It looks like the prevalent thing is to use INT32_MIN from <stdint.h> instead of std::numeric_limits. I know I pointed you at std::numeric_limits, but I think we should follow what other parts of LLVM use.

morisset updated this revision to Diff 14537.Oct 7 2014, 4:26 PM

Apply jfb suggestions.

jfb accepted this revision.Oct 7 2014, 4:32 PM
jfb edited edge metadata.

lgtm

This revision is now accepted and ready to land.Oct 7 2014, 4:32 PM
morisset closed this revision.Oct 7 2014, 5:04 PM
morisset updated this revision to Diff 14538.

Closed by commit rL219257 (authored by @morisset).