This is an archive of the discontinued LLVM Phabricator instance.

[TargetLowering][RISCV] Don't turn (seteq/ne (sext_inreg X, VT), C1) -> (seteq/ne (zext_inreg X, VT), C1) if the sext_inreg is cheaper
ClosedPublic

Authored by craig.topper on Jan 23 2021, 8:59 AM.

Details

Summary

RISCV has to use 2 shifts for (i64 (zext_inreg X, i32)), but we
can use addiw rd, rs1, x0 for sext_inreg. We already understood this
when type legalizing i32 seteq/ne on rv64. But this transform in
SimplifySetCC would sometimes undo it.

Diff Detail

Event Timeline

craig.topper created this revision.Jan 23 2021, 8:59 AM
craig.topper requested review of this revision.Jan 23 2021, 8:59 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 23 2021, 8:59 AM
Herald added a subscriber: MaskRay. · View Herald Transcript
frasercrmck added inline comments.Jan 25 2021, 2:33 AM
llvm/test/CodeGen/RISCV/double-stack-spill-restore.ll
22

I'm not familiar with this syntax: is this just a result of running the update_llc_test_checks.py script, and given that call func matches call func@plt it was passing anyway?

jrtc27 added inline comments.Jan 25 2021, 2:34 AM
llvm/test/CodeGen/RISCV/double-stack-spill-restore.ll
22

Yes, these check lines just predate full PIC support

luismarques accepted this revision.Jan 25 2021, 1:50 PM
luismarques added a subscriber: mundaym.

Makes sense to me.

@craig.topper are you using the correct clang-format config? Remember to fix the minor formatting issue.

llvm/test/CodeGen/RISCV/double-stack-spill-restore.ll
22

What confuses me about this is that @mundaym had fairly recently regenerated all of the tests, which added spill comments and @plt stuff. Why didn't update_llc_test_checks.py add the @plt here then?

This revision is now accepted and ready to land.Jan 25 2021, 1:50 PM

Makes sense to me.

@craig.topper are you using the correct clang-format config? Remember to fix the minor formatting issue.

I omitted that clang-format change from my patch because its not really in code I touched. Does clang-format widen its scope to include an else if that I that follows the one I changed?

jrtc27 added inline comments.Jan 25 2021, 2:02 PM
llvm/test/CodeGen/RISCV/double-stack-spill-restore.ll
22

Hm, I would guess 2047c10c22b071cccc57a7e2779d6603512e9113. Adding dso_local to func should make this diff go away, though I don't care which change is made, both are correct and end up testing what we want.

I omitted that clang-format change from my patch because its not really in code I touched. Does clang-format widen its scope to include an else if that I that follows the one I changed?

Oh yeah, nevermind. I had seen your comment (in another review) about your local clang-format giving different results than the clang-format in CI and I jumped the gun when I saw this lint check, assuming it had happened again. I don't know how loose git-clang-format is about formatting only the lines you actually changed.

This revision was landed with ongoing or failed builds.Jan 25 2021, 4:37 PM
This revision was automatically updated to reflect the committed changes.