[all-commits] [llvm/llvm-project] 45084e: [clang] Fix some clang->llvm type cache invalidati...

[aeubanks via All-commits]

  Branch: refs/heads/main
  Home:   https://github.com/llvm/llvm-project
  Commit: 45084eab5e63550bf2fdbf325d8fa5498263130d
      https://github.com/llvm/llvm-project/commit/45084eab5e63550bf2fdbf325d8fa5498263130d
  Author: Arthur Eubanks <aeubanks at google.com>
  Date:   2022-02-07 (Mon, 07 Feb 2022)

  Changed paths:
    M clang/lib/CodeGen/CGBuilder.h
    M clang/lib/CodeGen/CGCall.cpp
    M clang/lib/CodeGen/CodeGenTypes.cpp
    A clang/test/CodeGenCXX/type-cache-2.cpp
    A clang/test/CodeGenCXX/type-cache-3.cpp
    A clang/test/CodeGenCXX/type-cache.cpp

  Log Message:
  -----------
  [clang] Fix some clang->llvm type cache invalidation issues

Take the following as an example

  struct z {
    z (*p)();
  };

  z f();

When we attempt to get the LLVM type of f, we recurse into z. z itself
has a function pointer with the same type as f. Given the recursion,
Clang simply treats z::p as a pointer to an empty struct `{}*`. The
LLVM type of f is as expected. So we have two different potential
LLVM types for a given Clang type. If we store one of those into the
cache, when we access the cache with a different context (e.g. we
are/aren't recursing on z) we may get an incorrect result. There is some
attempt to clear the cache in these cases, but it doesn't seem to handle
all cases.

This change makes it so we only use the cache when we are not in any
sort of function context, i.e. `noRecordsBeingLaidOut() &&
FunctionsBeingProcessed.empty()`, which are the cases where we may
decide to choose a different LLVM type for a given Clang type. LLVM
types for builtin types are never recursive so they're always ok.

This allows us to clear the type cache less often (as seen with the
removal of one of the calls to `TypeCache.clear()`). We
still need to clear it when we use a placeholder type then replace it
later with the final type and other dependent types need to be
recalculated.

I've added a check that the cached type matches what we compute. It
triggered in this test case without the fix. It's currently not
check-clang clean so it's not on by default for something like expensive
checks builds.

This change uncovered another issue where the LLVM types for an argument
and its local temporary don't match. For example in type-cache-3, when
expanding z::dc's argument into a temporary alloca, we ConvertType() the
type of z::p which is `void ({}*)*`, which doesn't match the alloca GEP
type of `{}*`.

No noticeable compile time changes:
https://llvm-compile-time-tracker.com/compare.php?from=3918dd6b8acf8c5886b9921138312d1c638b2937&to=50bdec9836ed40e38ece0657f3058e730adffc4c&stat=instructions

Fixes #53465.

Reviewed By: rnk

Differential Revision: https://reviews.llvm.org/D118744