Page MenuHomePhabricator

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

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

Details

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.Aug 6 2019, 5:48 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 6 2019, 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
14131–14132
// xor (xor %a, %b), -1
spatel added inline comments.Aug 7 2019, 3:57 AM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
14101–14104

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

14131–14132

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.Aug 7 2019, 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.Aug 7 2019, 11:19 PM
rogfer01 added inline comments.
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
14101–14104

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.Aug 8 2019, 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.Aug 8 2019, 12:02 AM
rogfer01 added inline comments.
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
14101–14104

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

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

LGTM

This revision is now accepted and ready to land.Aug 8 2019, 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.Aug 8 2019, 11:11 PM
rogfer01 marked an inline comment as done.

ChangeLog:

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

Why is this one-use restriction being introduced here?

rogfer01 marked an inline comment as done.Aug 30 2019, 5:49 AM
rogfer01 added inline comments.
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
14104–14105

There are two callers of this function visitSETCC, which passes a node generated by SimplifySetCC, and visitBRCOND which does indeed check that the passed node has just one use.

I'm less sure about the the node coming out of SimplifySetCC so it might make sense to err on the safe side here?

lebedev.ri added inline comments.Aug 30 2019, 5:59 AM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
14104–14105

(*N->use_begin())->getOpcode() == ISD::BRCOND) should go, or make it an assert.

I'm not really sure what's on the other unsafe side, against what is this protecting?

rogfer01 updated this revision to Diff 256951.Apr 13 2020, 3:21 AM

ChangeLog:

  • Remove new hasOneUse and assert opcode of operand.

(Apologies it took me so long to get back to this)

rogfer01 marked 3 inline comments as done.Apr 13 2020, 3:23 AM
rogfer01 added inline comments.
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
14104–14105

@lebedev.ri @spatel I removed the "one use" check but I'm under the impression that we still want the assert, do we?

lebedev.ri added inline comments.Apr 13 2020, 4:57 AM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
14104–14105

Hm, no, i would think the assert should go away too.
Since there's no longer one-use check, we can't assume that ISD::BRCOND is he first user.
And i'm not sure against what we'd guarding with that check.

lenary added a comment.Jul 9 2020, 7:15 AM

Ping? This is mentioned in a RISC-V bug on llvm bugzilla, and seems like a nice improvement to multiple targets if we can land it.

@rogfer01 reverse-ping?
I'm not sure why this got stuck. Perhaps the code is fine and only the assert should go.

rogfer01 marked an inline comment as done.Jul 13 2020, 8:30 AM

Apologies, I totally forgot about this one. I'll update it ASAP.

rogfer01 updated this revision to Diff 277716.Jul 14 2020, 2:11 AM

ChangeLog:

  • Remove assert
asb added a comment.Jul 14 2020, 10:53 PM

This looks good to land IMHO. It's a nice improvement for RISC-V, and it seems to have been sufficiently reviewed by people active with the DAGCombiner. Thanks Roger.

lebedev.ri accepted this revision.Jul 15 2020, 12:01 AM

LG, i guess we'll deal with fallout if there's something missed.

Thanks all for the review, I'll land this shortly.

This revision was automatically updated to reflect the committed changes.