This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Add DAG combine for CTTZ/CTLZ in the case of input 0
ClosedPublic

Authored by djtodoro on May 25 2023, 7:26 AM.

Details

Summary

Within the AggressiveInstCombine Pass we have an analysis/optimization that matches that pattern of the Table Based CTZ - CTTZ algorithm. Some Targets do not support/define ctz(0), but since the AggressiveInstCombine is just an extension of InstCombine, it should be a target-independent canonicalization Pass, and therefore, we decided to introduce several instructions, such as select and compare that produce canonical IR, even if the input is 0. The task for the Targets that do support that input is to handle such a case and produce an optimal assembly.

This patch optimizes the CTTZ/CTLZ instructions if the input is 0 by performing the`DAG combine`, by generating the cttz(x) & 0x1f pattern (the same goes for ctlz as well).

Diff Detail

Event Timeline

djtodoro created this revision.May 25 2023, 7:26 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 25 2023, 7:26 AM
djtodoro requested review of this revision.May 25 2023, 7:26 AM
djtodoro edited the summary of this revision. (Show Details)May 25 2023, 7:26 AM
reames requested changes to this revision.May 25 2023, 1:23 PM
reames added a subscriber: reames.

Please provide a meaningful change description.

This revision now requires changes to proceed.May 25 2023, 1:23 PM

Please provide a meaningful change description.

Yeah, definitely needed. Sorry I messed it. Coming asap. Thanks!

djtodoro planned changes to this revision.May 26 2023, 7:34 AM
djtodoro updated this revision to Diff 526412.May 29 2023, 1:41 AM
djtodoro edited the summary of this revision. (Show Details)
  • Change/improve the summary
  • Improve the code
  • Reduce the test
craig.topper added inline comments.May 30 2023, 2:06 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
12528

This is unnecessary. If it is a ISD::SELECT it has exactly 3 operands.

12542

This is unnecesary

12555

Not sure anything above gurantees this. ISD::CTTZ and ISD::CTTZ_ZERO_UNDEF are valid for any type.

12558

the getNumOperands check is unnecessary. ISD::SETCC can only have exactly 3 operands.

12562

Drop curly braces

12585

You can use getZExtOrTrunc here to simplify this.

llvm/test/CodeGen/RISCV/cttz_zero_return_test.ll
3 ↗(On Diff #526412)

drop dso_local and local_unnamed_addr throughout the test.

craig.topper added inline comments.May 30 2023, 2:09 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
12547

Do your tests cover the CTZW case? That node doesn't exist until after type legalization, but I suspect all your tests match before type legalization.

djtodoro marked 8 inline comments as done.Jun 2 2023, 1:15 AM
djtodoro added inline comments.
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
12547

Hm, you are right, I m removing it for now.

12585

Oh yes, thanks!

djtodoro updated this revision to Diff 527773.Jun 2 2023, 1:17 AM
djtodoro marked 2 inline comments as done.
  • addressing comments
craig.topper added inline comments.Jun 2 2023, 9:56 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
12548

!isNullConstant(Op2)

12559

!isNullConstant(Op1->getOperand(1). We don't need the temporary variable Zero

12572

If the CTTZ opcode is CTTZ_ZERO_UNDEF, we need to change it to CTTZ.

djtodoro updated this revision to Diff 528096.Jun 3 2023, 4:36 AM
djtodoro marked 3 inline comments as done.
  • addressing comments
craig.topper added inline comments.Jun 5 2023, 9:59 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
12567

No underscores in variable names.

craig.topper added inline comments.Jun 5 2023, 10:04 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
12533
SDValue CTTZ = N->getOperand(2);
if (CTTZ.getOpcode() == ISD::TRUNCATE || CTTZ.getOpcode() == ISD::ZERO_EXTEND)
  CTTZ = CTTZ.getOperand(0);
12553

If we're requiring Op2 to be the null constant and Op3 to be the cttz then the only valid CC here is SETEQ. IsIntEquality checks for SETEQ or SETNE.

We should handle X != 0 ? CTTZ : 0 as well

mgudim added a comment.Jun 8 2023, 1:49 PM

see DAGCombiner::SimplifySelectCC for the logic of determining which operand is what. That code also handles CTLZ and case of reversed operands.

llvm/lib/Target/RISCV/RISCVISelLowering.cpp
12530

More descriptive names of operands would be better here. For example, Op1 is Cond, Op2 is ValOnZero, etc

12545

what if select is reversed?

mgudim added inline comments.Jun 8 2023, 1:52 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
12533

can you please give the example IR when we see ZERO_EXTEND or TRUNCATE?

djtodoro marked 6 inline comments as done.Jun 20 2023, 1:24 AM
djtodoro added inline comments.
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
12533

Please take a look into the functions: ctz_dereferencing_pointer_zext and ctz4 from the test.

12545

Done. Covered that as well.

12553

Yeah, I've handled the case that LLVM opt produces as a result of pattern matching, but yeah, definitely we should handle that case as well.

djtodoro updated this revision to Diff 532820.Jun 20 2023, 1:29 AM
djtodoro marked 3 inline comments as done.
  • Handle X != 0 ? CTTZ : 0

This also works for CTLZ.

djtodoro updated this revision to Diff 534441.Jun 26 2023, 12:14 AM
  • address comment
djtodoro retitled this revision from [RISCV] Add DAG combine for CTTZ in the case of input 0 to [RISCV] Add DAG combine for CTTZ/CTLZ in the case of input 0.Jun 26 2023, 12:17 AM
djtodoro edited the summary of this revision. (Show Details)

@djtodoro Thanks for working on this. LGTM, given that you fix the tests. I would feel more comfortable if someone else (@asb, @craig.topper, @reames) gave a final approval.

llvm/lib/Target/RISCV/RISCVISelLowering.cpp
12532

Suggestion: I would use Count or CountZeroes. On ARM CTZ means CTTZ.

craig.topper added inline comments.Jun 26 2023, 12:46 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
12530

eiher -> either

12532

I agree. ctz is also the trailing zero instruction on RISC-V.

12552

Do we need to guard against truncating to a type that can't represent the entire bitwidth. Or does that just workout because the AND we create would be all ones in that case?

12562

No need for LHS just put Cond->getOperand(0) into the if

12568

Do we need a new variable? Can we assign over CTZ for the _UNDEF cases?

12593

This if is unnecessary, getZExtOrtrunc will take care of it.

djtodoro marked 6 inline comments as done.Jun 29 2023, 1:36 AM
djtodoro added inline comments.
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
12552

Yeah, I'd say that the AND will work it out, and I guess it is ok to rely on that.

12568

Actually no, it does not improve readability. I will remove it.

djtodoro updated this revision to Diff 535674.Jun 29 2023, 1:37 AM
djtodoro marked an inline comment as done.
  • addressing comments
asb added a comment.Jul 9 2023, 9:58 AM

Just some feedback on the test case:

  • It doesn't show up in for RV64ZBB, but best to add the nounwind attribute for all functions
  • You should have an RV32ZBB RUN line too
  • I believe codegen differences are also visible for some of these tests without zbb, which I think is a good reason for including baseline RV32I and RV64I RUN lines too (and these will have unwanted .cfi_* noise if you don't add nounwind).
asb added a comment.Jul 9 2023, 11:07 PM

I forgot to also say that it's best to strip attributes that don't influence codegen. In this test case, noundef, nocapture and readonly don't alter the output if removed, so best to just get rid of them.

Reverse ping. Can we get this committed before the llvm 17 branch next week?

@asb Thanks for your comments! I will apply them during committing.

Reverse ping. Can we get this committed before the llvm 17 branch next week?

Sure, I will try to commit it later today.

This revision was not accepted when it landed; it landed in state Needs Review.Jul 19 2023, 7:23 AM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.