[PATCH] D107580: [VectorCombine] Support AND/UREM indices that require freezing.

Fri Aug 13 13:58:37 PDT 2021

fhahn marked 2 inline comments as done.
fhahn added inline comments.

================
Comment at: llvm/lib/Transforms/Vectorize/VectorCombine.cpp:779
+/// if a freeze needs to be inserted.
+class ScalarizationResult {
+  enum class StatusTy { Unsafe, Safe, SafeWithFreeze };
----------------
aqjune wrote:
> spatel wrote:
> > Other than the name, this looks like it could be a generic freeze utility. Should we put it in some header for other potential callers? cc @aqjune  @hyeongyukim 
> Not sure whether this class can be reused, but I agree that having a header containing utilities for dealing with freeze is a good idea (or probably reuse Local.h?).
> It's because we might want to keep freezes in a specific pattern for easier optimization. Currently, it is InstCombine's work to keep freeze-including expressions in a specific pattern, but it is kind of hidden.
> In the case of LoopUnswitch, naively freezing the branch condition caused many assembly diffs.
> To resolve this regression, we are experimenting with (1) pushing freeze into icmp arguments (which could resolve assembly diffs a lot: https://reviews.llvm.org/D106041#2922020) (2) replacing all uses of a branch condition with the frozen one.
> Once the scheme is set, I'd like to have FreezeBrCond(BranchInst) that performs what LoopUnswitch is supposed to do.
> 
> Other than the name, this looks like it could be a generic freeze utility. Should we put it in some header for other potential callers? cc @aqjune @hyeongyukim

I tried to keep things as general as possible here. But for now it serves a very specific use case, so maybe it would make sense to wait with moving this to a header until there are similar use cases?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107580