Page MenuHomePhabricator

[AggressiveInstCombine] fixed PR36121
ClosedPublic

Authored by aaboud on Jan 28 2018, 7:17 AM.

Details

Summary

Fixed the handle of trunc instruction as a leaf for expression pattern dominated by another trunc instruction.
In this case we need to update the work list of trunc instructions as follows:

  1. Replace old-trunc instruction node with the new-trunc instruction, if we reduced trunc instruction to different trunc instruction.
  2. Remove old-trunc instruction node from work-list, if we reduced trunc instruction to non-trunc instruction (e.g., to a constant expression).
  3. Add new-trunc instruction node to work-list, if we reduced a non-trunc instruction (e.g., zext/sext) into trunc instruction.

This should solve PR36121.

Diff Detail

Repository
rL LLVM

Event Timeline

aaboud created this revision.Jan 28 2018, 7:17 AM
craig.topper added inline comments.Jan 30 2018, 10:10 AM
lib/Transforms/AggressiveInstCombine/TruncInstCombine.cpp
335 ↗(On Diff #131706)

Could these just be isa instead of dyn_cast? Doesn't look like you need a TruncInst to do any of the worklist changes.

test/Transforms/AggressiveInstCombine/trunc_multi_uses.ll
223 ↗(On Diff #131706)

patters->patterns

247 ↗(On Diff #131706)

patters->patterns

aaboud marked 2 inline comments as done.Jan 31 2018, 1:14 AM
aaboud added inline comments.
lib/Transforms/AggressiveInstCombine/TruncInstCombine.cpp
335 ↗(On Diff #131706)
/// List of all TruncInst instructions to be processed.
  SmallVector<TruncInst *, 4> Worklist;

Unless you are suggesting to change the definition of Worklist to be a list of Instructions, I am not sure I can get rid of the dyn_cast! (I will get a compilation error).

Saying that, I a prefer to keep Worklist as list of TruncInst, this way I can assume I always have a truncate instruction in this list without need to assert/check that explicitly!

craig.topper accepted this revision.Jan 31 2018, 8:34 AM

LGTM

lib/Transforms/AggressiveInstCombine/TruncInstCombine.cpp
335 ↗(On Diff #131706)

Sorry I searched too quickly and saw the definition of one of the other worklists in this file that uses Value*.

This revision is now accepted and ready to land.Jan 31 2018, 8:34 AM
This revision was automatically updated to reflect the committed changes.