This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Add a DAG combine for (czero_eq X, (xor Y, 1)) -> (czero_ne X, Y) if Y is 0 or 1.
ClosedPublic

Authored by craig.topper on Jul 14 2023, 12:14 PM.

Details

Summary

This is an alternative to D155288 that can handle other sources of
xori like FP compares. Unfortunately, it misses the i64 setge case
on RV32 in condops.ll.

Diff Detail

Event Timeline

craig.topper created this revision.Jul 14 2023, 12:14 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 14 2023, 12:14 PM
craig.topper requested review of this revision.Jul 14 2023, 12:14 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 14 2023, 12:14 PM
mgudim added inline comments.Jul 14 2023, 1:38 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
12983

Why not express this using pattern?

craig.topper added inline comments.Jul 14 2023, 1:39 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
12983

I'm not sure what you mean.

craig.topper added inline comments.Jul 14 2023, 1:42 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
12983

Do you mean an isel pattern in tablegen? I'd need a PatFrag or ComplexPattern to call MaskValueIsZero. So there doesn't seem to be any advantage.

mgudim added inline comments.Jul 14 2023, 1:50 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
12983

def : Pat<(XLenVT (riscv_czero_eqz GPR:$rs1, (xor GPR:$rc, 1))),

(CZERO_NEZ GPR:$rs1, GPR:$rc)>;
mgudim added inline comments.Jul 14 2023, 1:58 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
12983

right. Got it, thanks.

LGTM, but it seems weird to me that we have to re-implement such basic optimisations instead of generic code doing this. @craig.topper @asb I know I asked this is the call, but I still don't understand why we emit target-specific node for SELECT so early?

LGTM, but it seems weird to me that we have to re-implement such basic optimisations instead of generic code doing this. @craig.topper @asb I know I asked this is the call, but I still don't understand why we emit target-specific node for SELECT so early?

There isn’t a generic combine for this on select after type legalization. It depends on getBooleanContents to know the value for true. So I think it only exists for i1.

LGTM, but it seems weird to me that we have to re-implement such basic optimisations instead of generic code doing this. @craig.topper @asb I know I asked this is the call, but I still don't understand why we emit target-specific node for SELECT so early?

There isn’t a generic combine for this on select after type legalization. It depends on getBooleanContents to know the value for true. So I think it only exists for i1.

Let's pretend that "SELECT" was legal. (I commented out setOperationAction(ISD::SELECT, XLenVT, Custom). Run this code through llc (llc -march=riscv64 -O2)

define i64 @foo(i64 %x, i64 %y, i64 %u, i64 %v) {
  %c = icmp sge i64 %x, %y
  %select_ = select i1 %c, i64 %u, i64 %v
  ret i64 %select_
}

Generic DAG combine will remove the xor after legalization:

Legalized selection DAG: %bb.0 'foo:'
SelectionDAG has 17 nodes:
  t0: ch,glue = EntryToken
          t2: i64,ch = CopyFromReg t0, Register:i64 %0
          t4: i64,ch = CopyFromReg t0, Register:i64 %1
        t20: i64 = setcc t2, t4, setlt:ch
      t22: i64 = xor t20, Constant:i64<1> <- THIS XOR
      t6: i64,ch = CopyFromReg t0, Register:i64 %2
      t8: i64,ch = CopyFromReg t0, Register:i64 %3
    t11: i64 = select t22, t6, t8
  t13: ch,glue = CopyToReg t0, Register:i64 $x10, t11
  t14: ch = RISCVISD::RET_GLUE t13, Register:i64 $x10, t13:1


Optimized legalized selection DAG: %bb.0 'foo:'
SelectionDAG has 15 nodes:
  t0: ch,glue = EntryToken
        t2: i64,ch = CopyFromReg t0, Register:i64 %0
        t4: i64,ch = CopyFromReg t0, Register:i64 %1
      t20: i64 = setcc t2, t4, setlt:ch
      t8: i64,ch = CopyFromReg t0, Register:i64 %3
      t6: i64,ch = CopyFromReg t0, Register:i64 %2
    t23: i64 = select t20, t8, t6
  t13: ch,glue = CopyToReg t0, Register:i64 $x10, t23
  t14: ch = RISCVISD::RET_GLUE t13, Register:i64 $x10, t13:1

It is done by DAGCombiner.cpp, extractBooleanFlip which does the right thing depending on getBooleanContents

asb added a comment.Jul 17 2023, 1:03 PM

I'd started down this route but then took the path of D155288 due to the fact this won't catch the RV32 cases. Did you have thoughts on those?

@asb

Did you have thoughts on those?

I haven't looked yet. I am in favour of merging https://reviews.llvm.org/D155288 (maybe together with this patch) now and we can keep looking for a better solution.

asb accepted this revision.Jul 19 2023, 11:37 AM

How about we merge this and then continue the discussion about something like D155288 to handle the remaining cases. This change LGTM.

This revision is now accepted and ready to land.Jul 19 2023, 11:37 AM
This revision was landed with ongoing or failed builds.Jul 19 2023, 12:33 PM
This revision was automatically updated to reflect the committed changes.