[PATCH] D144848: [SCEV] Skip instrs with non-scevable types in forget[Loop,Value].

Wed Mar 1 11:48:40 PST 2023

fhahn marked 3 inline comments as done.
fhahn added a comment.

In D144848#4154876 <https://reviews.llvm.org/D144848#4154876>, @mkazantsev wrote:

> Should this be under `VerifySCEV `?

The main reason I didn't use `VerifySCEV` is that this would require a check of the `VerifySCEV` variable on each `verifyAllUsersClearedFromMap`, whereas with the current setup it is free for builds without assertions.

================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:8467
+    if (!isSCEVable(I->getType()))
+      continue;
+    assert(ValueExprMap.count(I) == 0 &&
----------------
mkazantsev wrote:
> mkazantsev wrote:
> > nikic wrote:
> > > Hm, isn't the verification supposed to omit this check?
> > I guess we can skip it. We can't have them in `ValueExprMap` anyways.
> Maybe even assert on that.
The issue is that non-SCEVable instructions can be operands for SCEVable instructions (e.g. a floating point instruction used by an fcmp). I think in all those cases, the SCEV-able instruction needs to be a SCEVUnknown and the SCEV def-use chain is broken and we should be able to stop invalidation for this instruction.

I updated the condition to more explicitly check for this.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144848