Page MenuHomePhabricator

[GlobalISel] Extend not_cmp_fold to work on conditional expressions
ClosedPublic

Authored by foad on Thu, Aug 27, 6:43 AM.

Diff Detail

Event Timeline

foad created this revision.Thu, Aug 27, 6:43 AM
Herald added a project: Restricted Project. · View Herald TranscriptThu, Aug 27, 6:43 AM
foad requested review of this revision.Thu, Aug 27, 6:43 AM

Tests missing

llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
2289–2290

This is only better if this is fed by a compare you can invert. This should probably not do this if it finds some other i1 def.

We also should probably check if the inverted condcode is legal, but I guess we're missing cond code legality infrastructure (and none of the targets with gisel now don't need it)

2292–2293

This is illegal, you can just assume these are registers

foad added inline comments.Thu, Aug 27, 6:55 AM
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
2289–2290

This is only better if this is fed by a compare you can invert.

We process the whole expression tree and return false if we find anything that is not an AND or OR or a comparison.

foad updated this revision to Diff 288319.Thu, Aug 27, 7:09 AM

Remove unnecessary check. Improve comments.

foad updated this revision to Diff 288356.Thu, Aug 27, 8:48 AM

Add some tests.

paquette added inline comments.Thu, Aug 27, 8:57 AM
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
2268–2276

A MIR-esque example would be handy here, just to make it explicit what's being matched.

2269

I don't think here's any reason to talk about an index I here; from the looks of it, the below loop can be a range-based for?

2278

Usually people assert that the result of MRI.getVRegDef(...) isn't null.

I'm not sure if that's actually necessary in this case though; I *think* that it can only be null when you have a physreg, but I'm not entirely sure.

2329

A MIR-esque example would be useful here too?

arsenm added inline comments.Thu, Aug 27, 9:00 AM
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
2278

getVRegDef should never fail for gMIR.

foad added inline comments.Thu, Aug 27, 9:18 AM
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
2269

No, it's slightly tricky. It can't be a range-based for because we push more stuff into RegsToNegate inside the loop. It's a worklist algorithm, but without using a separate worklist vector.

Why isn't this caught in InstCombine?

Why isn't this caught in InstCombine?

This is going to show up as branch conditions are swapped for control flow lowering

arsenm added inline comments.Thu, Aug 27, 10:43 AM
llvm/test/CodeGen/AArch64/GlobalISel/prelegalizercombiner-invert-cmp.mir
227

Should have some cases where the conditions are a G_TRUNC or some other not-compare source

foad updated this revision to Diff 289385.Wed, Sep 2, 3:22 AM

Rebase. Add a non-matching test case.

foad marked an inline comment as done.Wed, Sep 2, 3:29 AM

Why isn't this caught in InstCombine?

This is going to show up as branch conditions are swapped for control flow lowering

Right, the xor is introduced by IRTranslator::emitSwitchCase. Perhaps it could be optimized at that point, instead of later in a combiner pass, but I'm not sure how feasible that is. I guess at that point you can't do accurate hasOneNonDBGUse checks?

aemerson accepted this revision.Wed, Sep 2, 1:27 PM

LGTM.

llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
2247

SmallVectorImpl?

This revision is now accepted and ready to land.Wed, Sep 2, 1:27 PM