This is an archive of the discontinued LLVM Phabricator instance.

[X86] (0 - SetCC) | C -> (zext (not SetCC)) * (C + 1) - 1 if we can get a LEA out of it.
ClosedPublic

Authored by deadalnix on Aug 7 2022, 9:34 AM.

Details

Summary

This adresses various regression in D131260 , as well as is a useful optimization in itself.

Diff Detail

Event Timeline

deadalnix created this revision.Aug 7 2022, 9:34 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 7 2022, 9:34 AM
deadalnix requested review of this revision.Aug 7 2022, 9:34 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 7 2022, 9:34 AM
RKSimon added inline comments.Aug 7 2022, 9:45 AM
llvm/lib/Target/X86/X86ISelLowering.cpp
48437

Can you improve this comment? The codegen doesn't seem to pattern matching below.

48445

auto *CN =

deadalnix updated this revision to Diff 450646.Aug 7 2022, 9:53 AM

Update comment and use auto

deadalnix retitled this revision from [X86] (sext Cond) | C -> (zext (not Cond)) * (C + 1) - 1 if we can get a LEA out of it. to [X86] (0 - SetCC) | C -> (zext (not SetCC)) * (C + 1) - 1 if we can get a LEA out of it..Aug 7 2022, 9:54 AM
RKSimon added inline comments.Aug 8 2022, 5:27 AM
llvm/lib/Target/X86/X86ISelLowering.cpp
48439

Limit this to legal i32/i64 cases? Add test coverage for other types as well.

48455

Not sure if we should use X86ISD::MUL_IMM here?

deadalnix added inline comments.Aug 8 2022, 3:26 PM
llvm/lib/Target/X86/X86ISelLowering.cpp
48439

That make sense.

48455

combineSelectOfTwoContants uses a mul, so I did the same.

deadalnix updated this revision to Diff 450995.Aug 8 2022, 4:16 PM
  • Limit the transform to 32bits and 64bits.
  • Add 64 bits test cases.

Alive is happy with the correctness of the transform, should we land this or are there extra concerns to be addressed?

RKSimon accepted this revision.Aug 10 2022, 1:41 AM

LGTM

This revision is now accepted and ready to land.Aug 10 2022, 1:41 AM