Followup to D59363 which failed to handle the icmp(X,undef) -> isTrueWhenEqual case - similar to llvm::ConstantFoldCompareInstruction
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
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... |
llvm/test/CodeGen/PowerPC/p10-spill-crgt.ll | ||
---|---|---|
423 | I’m sure most are because it’s what bugpoint does |
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. |
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 |
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.....).
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.
llvm/test/CodeGen/PowerPC/p10-spill-crgt.ll | ||
---|---|---|
423 | I think we have to just ignore it |
Don't switch on undef...