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

[Jay Foad via Phabricator via llvm-commits]

foad 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;
----------------
Pierre-vh wrote:
> 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.
You can repeat something like this every time you want to forward-declare UniformityInfo:
```
template<typename T> class GenericSSAContext;
using SSAContext = GenericSSAContext<Function>;
template<typename T> class GenericUniformityInfo;
using UniformityInfo = GenericUniformityInfo<SSAContext>;
```
Obviously this is a bit of a mouthful, but it's probably closer in spirit to what we've done in the past in LLVM.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145918