[PATCH] D145918: [DAG/AMDGPU] Use UniformityAnalysis in DAGISel

[Pierre van Houtryve via Phabricator via llvm-commits]

Pierre-vh added inline comments.


================
Comment at: llvm/include/llvm/Analysis/UniformityAnalysis.h:23
 extern template class GenericUniformityInfo<SSAContext>;
-using UniformityInfo = GenericUniformityInfo<SSAContext>;
+struct UniformityInfo : public GenericUniformityInfo<SSAContext> {
+  using GenericUniformityInfo<SSAContext>::GenericUniformityInfo;
----------------
sameerds wrote:
> This looks a bit disturbing to me. And it is squarely outside the limits of my understanding of C++ data types. I am even surprised that a "class UniformityInfo" declaration did not just work.
> 
> What happens if you put the following in TargetLowering.h?
> 
> ```
> extern template class GenericUniformityInfo<SSAContext>;
> using UniformityInfo = GenericUniformityInfo<SSAContext>;
> ```
> 
> I would certainly recommend that compared to the current approach.
`class UniformityInfo` doesn't work:
```
[build] In file included from /home/pierre/work/trunk/llvm-project/llvm/lib/CodeGen/SelectionDAG/FunctionLoweringInfo.cpp:16:
[build] /home/pierre/work/trunk/llvm-project/llvm/include/llvm/Analysis/UniformityAnalysis.h:23:7: error: typedef redefinition with different types ('GenericUniformityInfo<llvm::SSAContext>' (aka 'GenericUniformityInfo<GenericSSAContext<llvm::Function>>') vs 'llvm::UniformityInfo')
[build] using UniformityInfo = GenericUniformityInfo<SSAContext>;
[build]       ^
[build] /home/pierre/work/trunk/llvm-project/llvm/include/llvm/CodeGen/FunctionLoweringInfo.h:38:7: note: previous definition is here
[build] class UniformityInfo;
[build]       ^
```

Your approach doesn't work unless the definition for SSAContext is visible I believe. A quick test with a forward declaration of both SSAContext and GenericUniformityInfo shows:
```
[build] /home/pierre/work/trunk/llvm-project/llvm/include/llvm/ADT/GenericCycleInfo.h:235:12: error: field has incomplete type 'llvm::SSAContext'
[build]   ContextT Context;
[build]            ^
[build] /home/pierre/work/trunk/llvm-project/llvm/include/llvm/CodeGen/SelectionDAG.h:75:7: note: forward declaration of 'llvm::SSAContext'
[build] class SSAContext;
[build]       ^
```

Note that the idiom I used isn't really a hack and doesn't change the nature of UniformityInfo (in any way that matters AFAIK), it just makes `UniformityInfo` forward-declarable using the `struct` keyword. 

Though I agree it's a bit weird to see so if we can find an alternative that doesn't involve adding new includes/5 lines of forward declarations for UniformityInfo then I'd love to use that instead.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145918