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

[Bill Wendling via Phabricator via llvm-commits]

void added a comment.

In D75799#1933603 <https://reviews.llvm.org/D75799#1933603>, @joerg wrote:

> The original code has a functional dependency between sz and bytes and whether they can be constant evaluated. But the code doesn't express that. I don't think we can enforce that in any sensible way. There are valid use cases after all where partial inlining would result in entirely sensible decisions, just think about the more typical case of __builtin_constant_p selecting between inline asm taking immediate operands and one taking register/memory operands. That's why I am saying that I consider it a lot more useful to provide reliable building blocks to express the dependency and make sure they work.


[Adding the code snippets below for reference.]

Okay I think I get what you're saying now. If both `sz` and `bytes` are constant, then we can evaluate the if conditional and DCE properly. If `sz` or `bytes` isn't constant then the if conditional can't be DCE'd and that's when we require `bytes` to be non-constant. (Note: We don't care about compiling at `-O0`. Linux as a whole can't compile without optimizations.) In your code, you check that explicitly in the if conditional. While Linux is being clever and letting the compiler do it for us.

I don't argue that your suggested replacement is more readable and probably more amenable to the compiler, I still think that what Linux is doing is okay.

  Original Code:
  
  _Bool check_copy_size(const void *addr, unsigned long bytes, _Bool is_source)
  {
          int sz = __builtin_object_size(addr, 0);
          if (__builtin_expect(!!(sz >= 0 && sz < bytes), 0)) {
                  if (!__builtin_constant_p(bytes))
                          copy_overwrite(sz, bytes);
                  else if (is_source)
                          __bad_copy_from();
                  else
                          __bad_copy_to();
                  return false;
          }
          check_object_size(addr, bytes, is_source);
          return true;
  }
  
  Your Suggested Replacement:
  
  if (!(__builtin_constant_p(sz) && __builtin_constant_p(bytes) && sz >= 0 && sz >= bytes) && __builtin_expect(!!(sz >= 0 && sz < bytes), 0)) {
      if (!__builtin_constant_p(bytes))
      ...
  }


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75799