[PATCH] D75799: [JumpThreading] Don't create PHI nodes with "is.constant" values

Wed Mar 18 19:00:38 PDT 2020

void added a comment.

In D75799#1930221 <https://reviews.llvm.org/D75799#1930221>, @efriedma wrote:

> I'm not really happy about imposing optimization "rule", which we have to follow for correctness, without some explanation in LangRef.  Hacking a specific pass without a general rule is just going to make things more messy in the future; some other pass will eventually trip over this, and we won't have any reference for how it should be fixed.
>
> Really, to preserve the semantics the kernel wants in general, we need some notion of a "region": when we have an if statement with a __builtin_constant_p condition, the body of that if statement is a region with special semantics. If we resolve a __builtin_constant_p, every use of a variable used as an input to the __builtin_constant_p must be constant-folded inside that region.  We can optimize within the region, we can clone the whole region, but we can't clone the entry point of the region without cloning the whole thing, and we can't merge parts of the region without merging the whole thing.
>
> I'm not sure how to actually represent that in LLVM IR in a way that can be uniformly queried by optimizations in a reasonable way.  I guess marking llvm.is.constant "convergent" partially solves the issue, but we still might have problems with optimizations that merge code.

This isn't about imposing a new "rule" on optimizations. It's preventing an optimization from changing the semantics of the program. At the point where jump threading decides to clone the region and make one edge constant and the other non-constant, it's violating llvm.is.constant's semantics. I'll make this more explicit in the language reference. But the upshot is that this isn't a hack, because of how llvm.is.constant is meant to work (i.e. to mimic builtin_constant_p). I don't think we should try to optimize around llvm.is.constant beyond DCE, which is really the main purpose of builtin_constant_p.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75799