[PATCH] D35267: Pass Divergence Analysis data to selection DAG to drive divergence dependent instruction selection

[Stanislav Mekhanoshin via Phabricator via llvm-commits]

rampitec added inline comments.


================
Comment at: lib/Target/AMDGPU/AMDGPUISelLowering.cpp:794
+            return true;
+          return DA->isDivergent(FLI->getValueFromVirtualReg(Reg));
+        }
----------------
alex-t wrote:
> alex-t wrote:
> > rampitec wrote:
> > > alex-t wrote:
> > > > rampitec wrote:
> > > > > !DA || DA->isDivergent(...)
> > > > > 
> > > > > You are using getAnalysisIfAvailable, so it can be missing.
> > > > if DA == nulptr in the case above we'd return ((bool)!DA) true?
> > > > 
> > > > maybe it's better return false for the targets that have no DA?
> > > > 
> > > > I mean " DA && DA->isDivegent()" if we have no DA we return false. In case we have, the returned value will be defined by the isDivergent result
> > > That is conservatively correct to return true. Presumably targets w/o DA will have no use of the bit anyway, but if they are it is dangerous to assume uniformness.
> > > targets w/o DA will have no use of the bit anyway, but if they are it is dangerous
> > 
> > Sounds a bit paranoid :)  I just noted that returning "true" for the target that has no divergence at all looks misleading.
> Moreover, targets that have no DA have neither overridden isSDNodeSourceOfDivergence and will never get here.
What if DA is invalidated and thus NULL, even for targets with divergence?


================
Comment at: lib/Target/AMDGPU/AMDGPUISelLowering.cpp:826
+bool 
+llvm::AMDGPUTargetLowering::isItrinsicSourceOfDivegence(unsigned IntrID) const
+{
----------------
It still duplicates implementation in AMDGPUTargetTransformInfo.


https://reviews.llvm.org/D35267