[PATCH] D40546: StructurizeCFG: Test for branch divergence correctly

[Nicolai Hähnle via Phabricator via llvm-commits]

nhaehnle marked an inline comment as done.
nhaehnle added inline comments.


================
Comment at: include/llvm/Analysis/DivergenceAnalysis.h:40
+  //
+  // Even if this function returns false, V may still be divergent when used
+  // in a different basic block.
----------------
alex-t wrote:
> Comment is not necessary. By definition:
> 
> instruction produces uniform result if and only if all it's operands are uniform and all the branches it control depend of  are uniform.
> 
> This comment assume that the person who uses Divergence Analysis has no idea of what it is.
Strongly disagree. This is the header/interface file, and the definition you cite appears nowhere in the code, so the comment is valuable. To elaborate:

> This comment assume that the person who uses Divergence Analysis has no idea of what it is.

Kind of, though I would rather say: the comment assumes that the person who uses DivergenceAnalysis has no **experience** with it, and that is entirely fair. Actually, I'd say the mere existence of this patch is evidence that the comment makes sense.

You see, this patch fixes a bug in StructurizeCFG, which only uses divergence analysis optionally, and may be used by targets where divergence is not an issue. So it is entirely reasonable that a person who has no experience at all with DivergenceAnalysis would work on StructurizeCFG in a way that happens to touch how it interacts with DivergenceAnalysis. This comment serves as a warning to someone like that.


================
Comment at: lib/Transforms/Scalar/StructurizeCFG.cpp:253
+      : RegionPass(ID),
+        SkipUniformRegions(SkipUniformRegions || ForceSkipUniformRegions) {
     initializeStructurizeCFGPass(*PassRegistry::getPassRegistry());
----------------
arsenm wrote:
> I think the flag should override the pass argument if explicitly specified. I think other passes do something like if .getNumOccurrences() > 0 use the flag ...
Done.


================
Comment at: test/Transforms/StructurizeCFG/AMDGPU/uniform-regions.ll:1
+; RUN: opt -mtriple=amdgcn-- -S -o - -structurizecfg -structurizecfg-skip-uniform-regions < %s | FileCheck %s
+
----------------
arsenm wrote:
> I think we should start using update_test_checks for any structurer tests
Done


================
Comment at: test/Transforms/StructurizeCFG/uniform-regions.ll:1
+; RUN: opt -mtriple=amdgcn-- -S -o - -structurizecfg -structurizecfg-skip-uniform-regions < %s | FileCheck %s
+
----------------
arsenm wrote:
> The -mtriple should be dropped. If it's needed, you should create an AMDGPU specific test subdirectory in StructurizeCFG
Done.


================
Comment at: test/Transforms/StructurizeCFG/uniform-regions.ll:34
+  %v = load i32, i32 addrspace(2)* %gep, align 4
+  %cc2 = icmp eq i32 %v, 0
+  br i1 %cc2, label %end.loop, label %for.end
----------------
alex-t wrote:
> This is misleading. I can guess that using %v in the comparison means that branch condition is divergent.
> Nevertheless, I think that it's more clear if you explicitly use the divergent definition for %v.
> like:
> 
> %v = call i32 @llvm.amdgcn.workitem.id.x()
Done.


Repository:
  rL LLVM

https://reviews.llvm.org/D40546