Page MenuHomePhabricator

[AArch64] Peephole rule to remove redundant cmp after cset.

Authored by ilinpv on Mar 12 2021, 3:34 PM.



Comparisons to zero or one after cset instructions can be safely
removed in examples like:

cset w9, eq          cset w9, eq
cmp  w9, #1   --->   <removed>    .L1    .L1

cset w9, eq          cset w9, eq
cmp  w9, #0   --->   <removed>    .L1          b.eq    .L1

Peephole optimization to detect suitable cases and get rid of that
comparisons added.

Diff Detail

Event Timeline

ilinpv created this revision.Mar 12 2021, 3:34 PM
ilinpv requested review of this revision.Mar 12 2021, 3:34 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 12 2021, 3:34 PM
ilinpv edited the summary of this revision. (Show Details)Mar 12 2021, 3:39 PM

I see that this is an extension to the existing peephole optimizations, but is there any reason not to do this during ISel?

I see that this is an extension to the existing peephole optimizations, but is there any reason not to do this during ISel?

This optimization relies on liveness analysis to check that condition flags are not used in successors (see areCFlagsAliveInSuccessors) and I guess it would not fit ISel.

paquette added inline comments.Mar 15 2021, 10:57 AM
  • The name here is a little misleading. This is intended only for Bcc + select instructions. Maybe findCondCodeUseOperandIdxForBranchOrSelect?
  • This could use a Doxygen comment describing what it does.

This could use a doxygen comment documenting what the function is for.

It'd also be nice to document the parameters in the Doxygen comment as well.


If you return an Optional here you can still treat it like a bool, but also avoid passing UsedNZCV as an out-parameter.


I don't think these should ever be nullptr?


I think I would prefer an Optional<ArrayRef<MachineInstr *>> here if possible. I think that if someone were to use this and accidentally pass nullptr, it would be a mistake. Using an Optional better communicates that this is an optional parameter.


Pull CmpInstr->getParent() out into its own variable?


I don't think this actually does anything? It's writing to a local variable.


I think it'd be good to break this sentence down a little.

\returns True if \p CmpInstr can be removed.

\p IsInvertCC is true if, after removing \p CmpInstr, uses of the condition code must be inverted.

I don't think it's necessary to enumerate every case in this comment. It may fall out of sync if someone updates the code.


These should never be nullptr according to the asserts, so they might as well be passed by reference.


Use ArrayRef?


Pull isSUBSRegImm out into a variable and use it in both of the if statements?


Since MRI should never be nullptr, I think it makes sense for it to be a reference.


This assert could use a string explaining it.


Nit: static_cast is easier to grep for


Nit: You can use to generate the check lines. This makes it easier to update the test if necessary.

I imagine that the cset; cmp from the fp16 tests is just a missing optimization in ISel, and there might be good reason to do so there if it helps simplify the graph.

But like you said, this does fit in with the other peepholing code. And if it helps both ISels, that is good.

ilinpv updated this revision to Diff 331373.Mar 17 2021, 2:18 PM

Work on comments.

ilinpv marked 10 inline comments as done.Mar 17 2021, 2:49 PM
ilinpv added inline comments.

I am not sure about ArrayRef and const in examineCFlagsUse. The number of CCUseInstrs is defined in the analysis, instructions from CCUseInstrs can be later modified inverting condition code, CmpInstr can be removed.


Goog catch! Lost * after code refactoring. Test for that added.


I was inspired by canInstrSubstituteCmpInstr doxygen comments. I guess it should be clear what function is doing. Does it make sense to leave general doxygen comment for canCmpInstrBeRemoved, but put detailed ordinary comments inside function plus doxygen comments for examineCFlagsUse?

paquette added inline comments.Mar 23 2021, 10:27 AM

Minor nit: Doxygenification


We may want to assert that this is an immediate.


I think that would be good.

ilinpv updated this revision to Diff 333154.Mar 24 2021, 3:25 PM

Doxygen & comments update.

ilinpv marked 2 inline comments as done.Mar 24 2021, 3:29 PM
ilinpv added inline comments.

getImm() has an assert inside.

Just a friendly ping. Jessica @paquette, do you have any further suggestions?

dmgreen accepted this revision.Apr 15 2021, 1:34 AM

I think I managed to convince myself that this is correct. But there is a lot that could go wrong and subtly forgotten, this code has been a bit error prone in the past. Hopefully now that it's gained some infrastructure that's less likely.

LGTM, but please wait a couple of days in case @paquette has more comments.


Add a message for the assert?


I would leave brackets on statements like this, if the inner part is fairly big.

This revision is now accepted and ready to land.Apr 15 2021, 1:34 AM