[PATCH] D143631: [LTO] Don't let InstCombine re-sink the vastly more expensive fdiv

Tue Feb 14 11:10:36 PST 2023

spatel added a comment.

In D143631#4125429 <https://reviews.llvm.org/D143631#4125429>, @sdesmalen wrote:

> Depending on the LICM pass being run after InstCombine feels a bit like a game of cat and mouse, where the former hoists out the reciprocal and the latter restores things to the canonical form. I worry that guarding this ordering with just a test is a bit fragile.

Agree.

> There also isn't much precedent for changing InstCombine to //not// do the simplification, as I only spotted one cases in InstCombine (InstCombinerImpl::visitGEPOfGEP) where it looked at LoopInfo to see if something is loop invariant. Additionally, if having `1/x` outside the loop is the canonical form, it still relies on LICM being run first and for LoopInfo to be available in InstCombine (which in my understanding is optional, depending on the passes run before it).

Agree again. But we do make some concessions about canonical form when we know an instruction really is going to be expensive on all targets (any type of div/rem).

> So should this functionality perhaps be moved out of LICM into a more limited pass that is run just before SelectionDAG, or perhaps be made part of CodeGenPrepare?

CGP is an option, but that's also potentially not reliable (supposed to eventually go away with the switch to GISel?). A refinement of D87479 <https://reviews.llvm.org/D87479> doesn't seem that bad to me after all. It's a hack, but there's no ideal fix in sight.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D143631/new/

https://reviews.llvm.org/D143631