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

[Amara Emerson via Phabricator via llvm-commits]

aemerson added a comment.

Hi Quentin,

Thanks for taking a look.

> - Given this is canonicalization as opposed to optimization, shouldn't this be mandatory? I.e., if GISel runs, this runs. Alternatively, should we use another name not to imply this is mandatory (like beautification, I know this is ugly :P).

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.

> - Given this runs on IR, I assume we won't need something similar at the MIR level, is that correct?

Not that I can see. 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.

> - Given this runs on IR, I am guessing there are/will be some redundant code with InstCombine. Could/should we move the core of the logic in a separate library/file that could be called directly by this pass and InstCombine.

Perhaps, for this particular case it's less code to just do it manually since it's almost trivial. 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.

> - This kind of canonicalization could be useful to other ISels, should we make this a mandatory pass period? I.e., not tie to GISel at all.

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.

> Cheers,
> -Quentin


Repository:
  rL LLVM

https://reviews.llvm.org/D51953