This is an archive of the discontinued LLVM Phabricator instance.

[DAG] FoldSetCC - add missing icmp(X,undef) -> isTrueWhenEqual case
ClosedPublic

Authored by RKSimon on Aug 16 2023, 3:27 AM.

Details

Summary

Followup to D59363 which failed to handle the icmp(X,undef) -> isTrueWhenEqual case - similar to llvm::ConstantFoldCompareInstruction

Diff Detail

Event Timeline

RKSimon created this revision.Aug 16 2023, 3:27 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 16 2023, 3:27 AM
Herald added subscribers: luke, pmatos, asb and 30 others. · View Herald Transcript
RKSimon requested review of this revision.Aug 16 2023, 3:27 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 16 2023, 3:27 AM
nikic added a comment.Aug 16 2023, 3:36 AM

The transform looks fine to me, but a lot of these test cases need updates to remove undef (those that are bad reductions rather than intentional tests of undef).

llvm/test/CodeGen/AMDGPU/swdev373493.ll
57

Don't switch on undef...

llvm/test/CodeGen/PowerPC/p10-spill-crgt.ll
423

Don't switch on undef...

RKSimon added a subscriber: amyk.Aug 16 2023, 4:27 AM
RKSimon added inline comments.
llvm/test/CodeGen/PowerPC/p10-spill-crgt.ll
423

@amyk This is the original test case from D87705 - can you recall the background to why these tests use switch on undef?

arsenm added inline comments.Aug 16 2023, 4:29 AM
llvm/test/CodeGen/PowerPC/p10-spill-crgt.ll
423

I’m sure most are because it’s what bugpoint does

RKSimon added inline comments.
llvm/test/CodeGen/AMDGPU/swdev373493.ll
57

@yassingh This was added at D141895 - can this be replaced with the switch i32 %arg?

yassingh added inline comments.Aug 16 2023, 4:40 AM
llvm/test/CodeGen/AMDGPU/swdev373493.ll
57

This came up in that review too. The test in question was generated by a lot of automatic reductions over a larger failure, I couldn't get rid of undef then either.

arsenm added inline comments.Aug 17 2023, 5:58 AM
llvm/test/CodeGen/AMDGPU/swdev373493.ll
57

You need to replace it with a uniform value. If you just add a regular function argument, it will be divergent which is a much more radical change. The easiest update would probably be to swap the triple to amdpal from amdhsa, use amdgpu_gfx for the calling convention and make the new argument inreg

RKSimon added inline comments.Aug 21 2023, 9:15 AM
llvm/test/CodeGen/AMDGPU/swdev373493.ll
57

@yassingh Alternative to D158359 - could you run the original reduction again with this patch applied but D141895 disabled?

RKSimon added inline comments.Aug 22 2023, 5:33 AM
llvm/test/CodeGen/AMDGPU/swdev373493.ll
57

We've abandoned D158359 and intend to just let this change through

RKSimon added inline comments.Aug 22 2023, 7:07 AM
llvm/test/CodeGen/PowerPC/p10-spill-crgt.ll
423

Does anyone have any objections to the codegen change in test? Its likely that the new fold is affecting the test's usefulness (but that isn't that unusual for reduction) - ideally we'd re-run bugpoint with this patch in place, but I'm not sure if that's viable?

ping - neither the amdgpu or powerpc switch undef cases look to be necessary to fix (although it would have been nice if we could re-reduce them with this patch in place.....).

ping - neither the amdgpu or powerpc switch undef cases look to be necessary to fix (although it would have been nice if we could re-reduce them with this patch in place.....).

What is the motivation for this patch?

It was requested by @craig.topper for an upcoming patch - he noticed that D59363 had mentioned undef handling in the comments and hadn't implemented it.

It was requested by @craig.topper for an upcoming patch - he noticed that D59363 had mentioned undef handling in the comments and hadn't implemented it.

I see, I'll let craig review then because I'm not really sure what the implications/potential fallout of this are.

It was requested by @craig.topper for an upcoming patch - he noticed that D59363 had mentioned undef handling in the comments and hadn't implemented it.

I see, I'll let craig review then because I'm not really sure what the implications/potential fallout of this are.

I don't care if this gets fixed or not. I noticed it while looking at the test case in https://reviews.llvm.org/D157910 . The poison stores poisons the load which poisons the shift. Poison doesn't exist in SelectionDAG and becomes undef instead. The setcc ends up with undef. I was a little surprised it didn't get folded. Then I found the comment in FoldSetCC that said it should be.

We should either update the comment or move forward this patch. I don't necessarily care which.

arsenm accepted this revision.Sep 12 2023, 4:03 AM
arsenm added inline comments.
llvm/test/CodeGen/PowerPC/p10-spill-crgt.ll
423

I think we have to just ignore it

This revision is now accepted and ready to land.Sep 12 2023, 4:03 AM
This revision was landed with ongoing or failed builds.Sep 13 2023, 3:04 AM
This revision was automatically updated to reflect the committed changes.