This is an archive of the discontinued LLVM Phabricator instance.

[LLVM][Transforms] Zext flag in various optimization passes for RISC-V
Needs ReviewPublic

Authored by karouzakisp on Jul 27 2023, 9:06 AM.

Details

Reviewers
spatel
mkazantsev
Summary

Added zext flag for the target RISC-V,
in the passes that convert sext to zext because for RISC-V sext is always cheaper than zext.

Diff Detail

Event Timeline

karouzakisp created this revision.Jul 27 2023, 9:06 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 27 2023, 9:06 AM
karouzakisp requested review of this revision.Jul 27 2023, 9:06 AM

Please upload with full context. It's impossible to review most of these changes in the web view otherwise.

llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp
413

I don't think this is correct.

was_sext is supposed to indicate that the sign bit of the input to the zext is known to be 0. Making the zext and sext equivalent. In this case the input is only a single bit, making the input only a sign bit. It is not known to be 0.

karouzakisp retitled this revision from [LLVM][Transforms] to Zext flag in various optimization passes for RISC-V.Jul 27 2023, 9:47 AM

Added -U999999 in
git show
to make possible the reviewing via the web interface.

karouzakisp retitled this revision from Zext flag in various optimization passes for RISC-V to [LLVM][Transforms] Zext flag in various optimization passes for RISC-V.Jul 27 2023, 10:10 AM
craig.topper added inline comments.Jul 27 2023, 4:11 PM
llvm/lib/Transforms/InstCombine/InstCombineShifts.cpp
383

I don't think this is correct if we want the semantics of was_sext to be that the input to the zext has its sign bit clear.

This transform is based on the consuming instruction rather than anything about the producer.

1269

This change seems unnecessary. computeKnownBits should already be able to prove that lshr produces a positive number.

If we want to have this flag explicitly set anytime the sign bit of the input is 0, we should call isKnownNonNegative from visitZExt instead.

llvm/lib/Transforms/Scalar/BDCE.cpp
124

This is based on the consumer. That doesn't match the semantic we're trying to have of this bit giving information about the input.

llvm/lib/Transforms/Utils/SimplifyIndVar.cpp
552

I don't think this is correct for all cases that CanUseZExt returns true. Many of the cases are based on the consumer rather than the producer.

karouzakisp marked 3 inline comments as done.

Removed some redudant and inappropriate settings of the flag in the Transforms. Also updated the transforms calling method name from setWasSext to setNonNeg to set the flag.

Added nneg flag in InstCombine::visitZExt. Also Added nneg flag in CorrelatedValuePropagation::processZExt.