Page MenuHomePhabricator

[InferAddressSpaces] Add AS parameter to the pass factory
ClosedPublic

Authored by kpet on Apr 12 2019, 2:41 AM.

Details

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 <kevin.petit@arm.com>

Diff Detail

Repository
rL LLVM

Event Timeline

kpet created this revision.Apr 12 2019, 2:41 AM
kpet added a reviewer: joey.Apr 17 2019, 2:42 AM

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

include/llvm/Transforms/Scalar.h
383 ↗(On Diff #194822)

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

lib/Transforms/Scalar/InferAddressSpaces.cpp
631 ↗(On Diff #194822)

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

kpet added a comment.Apr 17 2019, 6:12 AM

@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 added inline comments.Apr 17 2019, 6:20 AM
include/llvm/Transforms/Scalar.h
383 ↗(On Diff #194822)

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 ↗(On Diff #194822)

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

kpet updated this revision to Diff 195556.Apr 17 2019, 6:32 AM

Address review comments.

kpet marked 3 inline comments as done.Apr 17 2019, 6:33 AM
arsenm accepted this revision.Apr 17 2019, 12:06 PM

LGTM

include/llvm/Transforms/Scalar.h
382 ↗(On Diff #195556)

\p AddressSpace

This revision is now accepted and ready to land.Apr 17 2019, 12:06 PM

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

kpet added a comment.Apr 18 2019, 5:05 AM

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

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 added a comment.EditedApr 18 2019, 6:18 AM

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 added a comment.Apr 18 2019, 6:26 AM

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.

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 added a comment.Apr 18 2019, 6:50 AM

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.

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

kpet added a comment.Apr 23 2019, 11:05 PM

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.

This revision was automatically updated to reflect the committed changes.