This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Have sexti32 also recognize AssertZExt from types smaller than i32.
ClosedPublic

Authored by craig.topper on Feb 20 2021, 4:33 PM.

Details

Summary

An i64 AssertZExt from a type smaller than i32 has at least 33
leading zeros which mean it has at least 33 sign bits.

Since we have a couple patterns that use two sexti32, I've
switched to a ComplexPattern so tablegen didn't have to generate
9 different permutations.

As noted in the FIXME, maybe we should just call computeNumSignBits,
but we don't have tests that benefit from that yet.

Diff Detail

Event Timeline

craig.topper created this revision.Feb 20 2021, 4:33 PM
craig.topper requested review of this revision.Feb 20 2021, 4:33 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 20 2021, 4:33 PM
Herald added a subscriber: MaskRay. · View Herald Transcript
craig.topper added inline comments.Feb 20 2021, 4:35 PM
llvm/lib/Target/RISCV/RISCVInstrInfoF.td
403

The i64 instead of GPR here tricks tablegen into not generating extra patterns for RV32.

LGTM though I'll defer my acceptance to @asb or @luismarques who have more to do with the non-vector side of things.

I do think it makes sense to do this in a ComplexPattern rather than have them all in the table. I've never liked how many forms of sext/zext/trunc there can be when doing pattern matching at this stage. I'm surprised there hasn't been an effort to unify it as it's often infeasible (or, easy to forget) to cover all the cases.

luismarques accepted this revision.Feb 22 2021, 2:12 PM

Makes sense to me.

llvm/lib/Target/RISCV/RISCVInstrInfoF.td
403

I think there would be a benefit in adding this as a code comment. In at least one of the tablegen files, if not all of them.

This revision is now accepted and ready to land.Feb 22 2021, 2:12 PM
jrtc27 added inline comments.Feb 22 2021, 2:17 PM
llvm/lib/Target/RISCV/RISCVInstrInfoF.td
403

Does (i64 GPR:$rs1) also achieve the same effect like is used on lines 405 and 406? That'd be nicer if it works IMO. Also I think it'd be worthwhile having zexti32 be consistent with sexti32 even if not strictly necessary; currently the difference is rather odd.

craig.topper added inline comments.Feb 22 2021, 2:51 PM
llvm/lib/Target/RISCV/RISCVInstrInfoF.td
403

That does work so I'll switch to that. I'll move zexti32 in a followup patch. It's a little weirder since we have to look at the mask on an AND and use computeKnownBits to match the behavior of OPC_CheckAndImm. I think we might be able to call into the code in SelectionDAGISel.cpp that handles that.