[PATCH] D51953: [GlobalISel] Add a new IR canonicalization pass

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet accepted this revision.
qcolombet added a comment.
This revision is now accepted and ready to land.

Hi Amara,

> Are you referring to the enable-gisel-canonicalize option? I can remove that to force it to always run when GISel does, it's just an old habit of mine to add toggle flags for new passes.

Yes, I was referring to this.
I think it depends what's the intent: is this required for correctness (always run) or simply yield better code.
If this is for correctness (say for instance because we conservatively declare that we don't want to handle weird cases), then we should have a way to enforce (verifier) it.

> There may be cases where we need to re-do canonicalization if legalizer breaks it, but as we discussed we should try to fix that in the offending passes if possible.

Sounds good to me.

> I think if we wanted to share the code in future then we'd need to create a new library, to prevent LLVMCodeGen having to depend on LLVMInstCombine which would be weird.

Agree.

> Maybe, but we don't have a real motivating use case right now. SelectionDAG already handles the compare immediate case already at -O0. -O0 canonicalizations is where I see this being most useful in order to be on par with the other ISels, and at higher opt levels we can rely on instcombine to do most of it.

FastISel may benefit from it as well, but I agree this is not strictly require plus we are killing fastisel, so not changing its inputs mean no need to do more validations :).

LGTM (one thing to think about is what we discuss regarding correctness vs. beautification, but that shouldn't hold this PR)

Cheers,
-Quentin


Repository:
  rL LLVM

https://reviews.llvm.org/D51953