Page MenuHomePhabricator

[DAGCombiner] Rebuild (setcc x, y, ==) from (xor (xor x, y), 1)
AcceptedPublic

Authored by rogfer01 on Tue, Aug 6, 5:48 AM.

Details

Reviewers
efriedma
spatel
Summary

The existing code already considered this case. Unfortunately a typo in the condition prevents it from triggering. Also the existing code, had it run, forgot to do the folding.

This fixes PR42876

Diff Detail

Event Timeline

rogfer01 created this revision.Tue, Aug 6, 5:48 AM
Herald added a project: Restricted Project. · View Herald TranscriptTue, Aug 6, 5:48 AM
rogfer01 set the repository for this revision to rG LLVM Github Monorepo.
lebedev.ri added inline comments.
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
13526–13527
// xor (xor %a, %b), -1
spatel added inline comments.Wed, Aug 7, 3:57 AM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
13495–13499

That comment makes me nervous given the logic. Can we assert that N has one user and that user is a br?

13526–13527

The code is checking for '1' not '-1'.
Can we assert that the types are i1 and/or the code should be changed to use llvm::isBitwiseNot()?

rogfer01 updated this revision to Diff 214067.Wed, Aug 7, 11:12 PM
rogfer01 retitled this revision from [DAGCombiner] Fold br(xor(xor(x, y), 1)) as br(x == y) to [DAGCombiner] Rebuild (setcc x, y, ==) from (xor (xor x, y), 1).

ChangeLog:

  • Tighten conditions in which the fold is executed
  • Use isBitwiseNot instead of checking for (xor e, 1)
  • Remove TheXor
  • Rephrase comments so they are less confusing by not mentioning br.
rogfer01 marked an inline comment as done.Wed, Aug 7, 11:19 PM
rogfer01 added inline comments.
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
13495–13499

I tried checking that the user is a br but then lots of other unrelated tests started failing.

I will look closer what is going on here.

rogfer01 updated this revision to Diff 214070.Thu, Aug 8, 12:01 AM

ChangeLog:

  • Tighten even more the condition in which the fold is applied
  • Rephrase comments so they mention brcond.
rogfer01 marked 3 inline comments as done.Thu, Aug 8, 12:02 AM
rogfer01 added inline comments.
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
13495–13499

I must have done something silly earlier because this seems OK.

spatel accepted this revision.Thu, Aug 8, 8:50 AM

LGTM

This revision is now accepted and ready to land.Thu, Aug 8, 8:50 AM

I don't think this is legal in general unless you restrict it to i1. See https://rise4fun.com/Alive/1Hyj .

rogfer01 updated this revision to Diff 214304.Thu, Aug 8, 11:11 PM
rogfer01 marked an inline comment as done.

ChangeLog:

  • Constrain fold to i1.
lebedev.ri added inline comments.Fri, Aug 9, 5:48 AM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
13496–13499

Why is this one-use restriction being introduced here?