[PATCH] D153992: AMDGPU: Avoid computing dominator tree for printf lowering

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUPrintfRuntimeBinding.cpp:505
+  auto GetDT = [&FAM](Function &F) -> DominatorTree * {
+    return FAM.getCachedResult<DominatorTreeAnalysis>(F);
   };
----------------
arsenm wrote:
> arsenm wrote:
> > nikic wrote:
> > > Cached analyses on the NewPM should only be used for analyses preservation. You should either require DT here or not use it at all.
> > I don't see anything about this in the comments on getCachedResult
> Is this not identical to how getBestSimplifyQuery uses this?
It is -- we should fix it. Thankfully the places it is used have all the relevant analyses available anyway so it makes no functional difference.

To clarify, the reason why this is problematic is that while the LegacyPM determined analysis availability statically, in the NewPM this depends on whether some pass happened to make any changes and ended up invalidating analyses or not. This means that we (generally speaking) do not have guarantees about whether a given analysis will be available at a specific pipeline position. You'll get the same pass executing with the analysis or without the analysis, at the same pipeline position, depending on how exactly your IR looks like.


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

https://reviews.llvm.org/D153992