Page MenuHomePhabricator

[InstSimplify] Fold abs(abs(x)) -> abs(x)
ClosedPublic

Authored by craig.topper on Jul 31 2020, 3:22 PM.

Details

Summary

It's always safe to pick the earlier abs regardless of the nsw flag. We'll just lose it if it is on the outer abs but not the inner abs.

Diff Detail

Event Timeline

craig.topper created this revision.Jul 31 2020, 3:22 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 31 2020, 3:22 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
craig.topper requested review of this revision.Jul 31 2020, 3:22 PM
arsenm added a subscriber: arsenm.Jul 31 2020, 4:23 PM

There's an isidempotent function somewhere that catches this

There's an isidempotent function somewhere that catches this

Yeah its called from visitUnaryIntrinsic. This intrinsic is binary because of the nsw argument. I could add it to IsIdempotentand add another call to isIdempotent to the top of visitBinaryIntrinsic, but I thought with the oddity of nsw handling abs directly might be more clear.

nikic accepted this revision.Aug 1 2020, 1:01 AM

LG. This has some overlap with D85043, which will already catch the cases where the inner abs has poison=true, but not those with poison=false, so this seems reasonable as an explicit fold. One could make an argument that this should live in InstCombine and try to preserve the outer poison flag, but this doesn't seem particularly important to me.

This revision is now accepted and ready to land.Aug 1 2020, 1:01 AM
spatel added a comment.Aug 1 2020, 7:03 AM

Will need update after:
rG04b99a4d18cf

llvm/lib/Analysis/InstructionSimplify.cpp
5262

Could use match here to make this slightly more obvious:

if (match(Op0, m_Intrinsic<Intrinsic::abs>(m_Value(), m_Value())))
  return Op0;
This revision was automatically updated to reflect the committed changes.