Page MenuHomePhabricator

[DAG] Fix constant store folding to handle non-byte sizes.

Authored by niravd on Feb 25 2019, 7:41 AM.

Diff Detail


Event Timeline

niravd created this revision.Feb 25 2019, 7:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 25 2019, 7:41 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
courbet added inline comments.Feb 25 2019, 8:04 AM
15436 ↗(On Diff #188177)

This is overly optimistic: for example, this will return true for STBitSize = 1 and ChainBitSize= 2 if both are at offset 0

What about passing size in bits to BaseIndexOffset::contains() and letting it do the computation in bits:

bool BaseIndexOffset::contains(int64_t BitSize, const BaseIndexOffset &Other,
                               int64_t OtherBitSize, const SelectionDAG &DAG,
                               int64_t &Offset) const {
  if (!equalBaseIndex(Other, DAG, Offset))
    return false;
  if (Offset >= 0) {
    // Other is after *this:
    // [-------*this---------]
    //            [---Other--]
    // ==Offset==>
    return 8 * Offset + OtherBitSize <= BitSize;
  // Other starts strictly before *this, it cannot be fully contained.
  //    [-------*this---------]
  // [--Other--]
  return false;
niravd updated this revision to Diff 188218.Feb 25 2019, 10:47 AM

Convert to bit-level sizing for contains. I'm leaving equalBaseIndex as byte-level for now, but a followup NFC patch is probably warranted.

courbet accepted this revision.Feb 25 2019, 11:51 PM
courbet added inline comments.
63 ↗(On Diff #188218)

please update the comment.

70 ↗(On Diff #188218)

BitOffset ?

This revision is now accepted and ready to land.Feb 25 2019, 11:51 PM

Nice! I ran a new bunch of llvm-stress tests with this patch and now I don't see the crashes anymore.
Please push.

Added a few nits about too long lines.

65 ↗(On Diff #188218)

long line

69 ↗(On Diff #188218)

long line

139 ↗(On Diff #188218)

long line

This revision was automatically updated to reflect the committed changes.