[PATCH] Add a fence elimination pass

Robin Morisset morisset at google.com
Fri Oct 17 14:30:05 PDT 2014


================
Comment at: include/llvm/Transforms/Scalar.h:44
@@ -30,1 +43,3 @@
+        std::function<bool(const Instruction&)> isWeakerFence
+            = [](const Instruction &I){return false;});
 
----------------
morisset wrote:
> jfb wrote:
> > I'm not a fan of default arguments, I think you shouldn't have them.
> > 
> > Can you add a description of what stronger/weaker mean, as well as FenceArgs?
> > 
> > `std::function` isn't in common use in the code base, can you see what similar functions use instead?
> > 
> > Overall you're trying to create a partial ordering for target-specific intrinsics for fences, and then manually adding this pass many times to a backend. Could you instead have the backend specify the ordering, and add the pass only once? The pass would handle each fence type that it's made aware of, in the proper strong->weak ordering.
> OK for removing the default argument.
> 
> For the ordering, I will try to see if having the backend pass an ArrayRef<> of the different fences, sorted by strength works. The main weakness is that it would make it a total ordering which is fine for most architectures but may fail for some (only Alpha which is not supported by LLVM out of my head, but I don't know every architecture out there). How could the backend cleanly specify a partial ordering ?
According to the LLVM programmer's manual, std::function is fine for closures if they are to be stored (like here). And they are used in a few places in the code.

An additional difficulty with using an ArrayRef, is that there are lots of information we may want to compare: the intrinsic ID, the operands to the intrinsic, and possibly metadata in the future. So it would be an ArrayRef of tuples of monstrous arity. Is it ok if I keep the current architecture at least for now ?

http://reviews.llvm.org/D5758






More information about the llvm-commits mailing list