This is an archive of the discontinued LLVM Phabricator instance.

[DAGCombiner] In foldSelectOfConstants, use 'sub C1+1, (zext Cond)' instead of 'add (sext Cond), C1+1'
AbandonedPublic

Authored by craig.topper on Jun 24 2018, 3:53 PM.

Details

Reviewers
spatel
RKSimon
Summary

On X86, we usually end up converting the add to the sub form anyway. On PowerPC, this seems to avoid some nots in some of the cases.

The changes are in both the select_const tests are regressions. The PowerPC regression was largely just getting lucky that the sext worked well with the AssertSExt of the input. The code is now similar to another test case that uses 'add (zext Cond), C1' where that doesn't work well with the AssertSExt.

I haven't looked into the X86 regression yet, but it looks like some sort of reassociation issue.

Diff Detail

Event Timeline

craig.topper created this revision.Jun 24 2018, 3:53 PM

@spatel, what are you thoughts on this?

@spatel, what are you thoughts on this?

Did we get the motivating cases with our other select patches? If not, can you post those? As-is, I don't see why we'd change this from these test diffs.

Yeah I think the other patches took care of my perf regression. I'm fine to abandon this if you don't think the PowerPC changes are worthwhile.

Yeah I think the other patches took care of my perf regression. I'm fine to abandon this if you don't think the PowerPC changes are worthwhile.

Yep, PPC has a lot of missed opportunities. That could probably be solved with better pattern matching later on. For x86, if cmov really is implemented as a single-cycle and full-throughput op, then we probably shouldn't be doing these kinds of folds anyway.

Also worth noting: no other in-tree targets have enabled the convertSelectOfConstantsToMath() hook to get this transform, but I think that's just an oversight. Not sure what differences we'd see if more targets override the default.

craig.topper abandoned this revision.Jun 28 2018, 8:52 PM