[InferAddressSpaces] Add AS parameter to the pass factory

[kpet]

Summary

This enables the pass to be used in the absence of
TargetTransformInfo. When the argument isn't passed,
the factory defaults to UninitializedAddressSpace and
the flat address space is obtained from the
TargetTransformInfo as before this change. Existing
users won't have to change.

Signed-off-by: Kevin Petit <[e-mail]>

[arsenm]

This is still requiring the TTI as a dependency, so I don't understand what this solves?

include/llvm/Transforms/Scalar.h

383

Parameter name should be more descriptive. The comment also needs to be updated

lib/Transforms/Scalar/InferAddressSpaces.cpp

631

Extra blank line here. Also should sink the getAnalysis into here

[kpet]

@arsenm Thanks for the review.

This is still requiring the TTI as a dependency, so I don't understand what this solves?

The TTI is currently used for two things:

• getFlatAddressSpace which is absolutely required to return a non-default value for the pass to be able to function
• hasVolatileVariant which enables the pass to handle additional cases. The default TTI implementation will return false which is good enough for many uses.

This change enables >95% of the functionality of this pass to be used with the default TTI.

[kpet]

include/llvm/Transforms/Scalar.h

383

Will do. I assumed that given that AddressSpace is shortened to AS in a lot of place, I could do it here as well.

lib/Transforms/Scalar/InferAddressSpaces.cpp

631

Will remove the line. Not sure moving getAnalysis into this block helps. It is still required as you pointed out.

[kpet]

Address review comments.

[arsenm]

LGTM

include/llvm/Transforms/Scalar.h

382

\p AddressSpace

[Anastasia]

Btw, how is this tested? Do we have an existing test that exercises this already?

[kpet]

The pass is used by the AMDGPU and NVPTX backends. All their tests are passing with this change.

[Anastasia]

In D60602#1471553, @kpet wrote:

The pass is used by the AMDGPU and NVPTX backends. All their tests are passing with this change.

Yes, but they were passing before? Or is this fixing any bug in the existing tests?

If the change is adding functionality it is a good practice to add a test demonstrating its use. Otherwise, someone can commit something later that break the functionality and we might not have any way to detect that it's broken.

[Anastasia]

In D60602#1471588, @Anastasia wrote:

In D60602#1471553, @kpet wrote:

The pass is used by the AMDGPU and NVPTX backends. All their tests are passing with this change.

Yes, but they were passing before? Or is this fixing any bug in the existing tests?

If the change is adding functionality it is a good practice to add a test demonstrating its use. Otherwise, someone can commit something later that break the functionality and we might not have any way to detect that it's broken.

PS, there are situations in which some functionality can't be tested and then it can be acceptable to commit w/o a test. But I don't know if this is the case. I am just asking whether it has been analyzed.

[kpet]

This doesn't fix anything broken in LLVM but I did check that it didn't break any of the existing tests either :).

I did think about adding a test but in this case testing the new parameter would require a full C++ app that makes use of the pass and grep'ing in the tests returned zero matches for similar tests.

[Anastasia]

In D60602#1471614, @kpet wrote:

This doesn't fix anything broken in LLVM but I did check that it didn't break any of the existing tests either :).

I did think about adding a test but in this case testing the new parameter would require a full C++ app that makes use of the pass and grep'ing in the tests returned zero matches for similar tests.

Ok, we don't add C++ tests to llvm. But would it be possible to extract an .ll file generated from C++ (may be with some amendments) and run opt with the pass enabled?

Then you could check for IR to be expected. Are you expecting a certain IR pattern to occur with your change?

[kpet]

But would it be possible to extract an .ll file generated from C++ (may be with some amendments) and run opt with the pass enabled?

I did consider this as well. My understanding is that it would require further changes to the pass to add a command-line parameter that could be a third source for the flat address space. It would be a good thing to have but it wouldn't test my change to the pass factory.

Are you expecting a certain IR pattern to occur with your change?

Not as such. This change is just plumbing.

[arsenm]

The AMDGPU usage could be changed to use this instead of TTI. There’s no real difference

[kpet]

The implementation of getFlatAddressSpace in the AMDGPU backend does a bit more than just return a constant. This logic would need to be duplicated or the TTI called from their pass factory which would be the first use. TBH, I'm not too worried about the change to the factory breaking. This will be used in an out-of-tree project that tracks llvm fairly closely. If this breaks, I'll propose a change to exercise this from some in-tree code.

include/llvm/Transforms/Scalar.h

 // ValuePropagation - Propagate CFG-derived value information
 //
 Pass *createCorrelatedValuePropagationPass();
 
 //===----------------------------------------------------------------------===//
 //
 // InferAddressSpaces - Modify users of addrspacecast instructions with values
 // in the source address space if using the destination address space is slower
 // on the target.
 //
-FunctionPass *createInferAddressSpacesPass();
+FunctionPass *createInferAddressSpacesPass(unsigned AS = ~0u);
 extern char &InferAddressSpacesID;
 
 //===----------------------------------------------------------------------===//
 //
 // LowerExpectIntrinsics - Removes llvm.expect intrinsics and creates
 // "block_weights" metadata.
 FunctionPass *createLowerExpectIntrinsicPass();
 

lib/Transforms/Scalar/InferAddressSpaces.cpp

 class InferAddressSpaces : public FunctionPass {
   /// Target specific address space which uses of should be replaced if
   /// possible.
   unsigned FlatAddrSpace;
 
 public:
   static char ID;
 
-  InferAddressSpaces() : FunctionPass(ID) {}
+  InferAddressSpaces() :
+    FunctionPass(ID), FlatAddrSpace(UninitializedAddressSpace) {}
+  InferAddressSpaces(unsigned AS) : FunctionPass(ID), FlatAddrSpace(AS) {}
 
   void getAnalysisUsage(AnalysisUsage &AU) const override {
     AU.setPreservesCFG();
     AU.addRequired<TargetTransformInfoWrapperPass>();
   }
 
   bool runOnFunction(Function &F) override;
 

 }
 
 bool InferAddressSpaces::runOnFunction(Function &F) {
   if (skipFunction(F))
     return false;
 
   const TargetTransformInfo &TTI =
       getAnalysis<TargetTransformInfoWrapperPass>().getTTI(F);
+
+  if (FlatAddrSpace == UninitializedAddressSpace) {
+
     FlatAddrSpace = TTI.getFlatAddressSpace();
     if (FlatAddrSpace == UninitializedAddressSpace)
       return false;
+  }
 
   // Collects all flat address expressions in postorder.
   std::vector<WeakTrackingVH> Postorder = collectFlatAddressExpressions(F);
 
   // Runs a data-flow analysis to refine the address spaces of every expression
   // in Postorder.
   ValueToAddrSpaceMapTy InferredAddrSpace;
   inferAddressSpaces(Postorder, &InferredAddrSpace);
bool InferAddressSpaces::rewriteWithNewAddressSpaces(
   }
 
   for (Instruction *I : DeadInstructions)
     RecursivelyDeleteTriviallyDeadInstructions(I);
 
   return true;
 }
 
-FunctionPass *llvm::createInferAddressSpacesPass() {
-  return new InferAddressSpaces();
+FunctionPass *llvm::createInferAddressSpacesPass(unsigned AS) {
+  return new InferAddressSpaces(AS);
 }