This is an archive of the discontinued LLVM Phabricator instance.

[Fortran Support] Add pattern match style for local fortran arrays
ClosedPublic

Authored by bollu on May 10 2017, 3:04 PM.

Details

Summary

Added another pattern match for a fortran array that is local.
This breaks the previous assumption that Fortray Arrays are GlobalValue.

The names of functions were getting a little unweildy, so I renamed the
Fortran related functions here.

WIP: I need to minimise the testcase

Diff Detail

Repository
rL LLVM

Event Timeline

bollu created this revision.May 10 2017, 3:04 PM
bollu updated this revision to Diff 98604.May 11 2017, 3:38 AM
  • simplify testcase
bollu updated this revision to Diff 98607.May 11 2017, 3:48 AM
  • [FIX] add correct annotated version of testcase
bollu added a comment.May 12 2017, 1:13 AM

@Meinersbur: ping, I am comfortable with this patch being reviewed now.

lib/Analysis/ScopBuilder.cpp
315 ↗(On Diff #98607)

renamed because the names were getting unwieldy, but I dislike homebrewed abbreviations like FAD. Do you have any naming suggestions?

371 ↗(On Diff #98607)

Once this is mainlined, I will probably go and add a GEP pattern matcher so I can simplify this code.

lib/Analysis/ScopInfo.cpp
1037 ↗(On Diff #98607)

be consistent between FortranArrayDescriptor and FAD? Find a new name which conveys the same information?

Meinersbur added inline comments.May 12 2017, 4:30 AM
include/polly/ScopBuilder.h
69 ↗(On Diff #98607)

If the name is too long, you can commit the shortening separately (no need for a review).

82 ↗(On Diff #98607)

Doxygen comments usually begin with a @brief line (one sentence only), followed by an empty /// line, followed by a detailed description. Sometime ago LLVM to drop the explicit @brief.

83 ↗(On Diff #98607)

Fortran, accesss, Succeeds

84 ↗(On Diff #98607)

has been allocated.

Dot at the end of sentence.

lib/Analysis/ScopBuilder.cpp
126 ↗(On Diff #98607)

Update doxygen comment.

136–137 ↗(On Diff #98607)

Update doxygen comment

146 ↗(On Diff #98607)

Suggestion: isFortranArray. I don't see what the "Value" part adds.

315 ↗(On Diff #98607)

extractFortranArrayDesc?

354–370 ↗(On Diff #98607)

The doxygen should be where the method is declared, not here where it is defined.

358 ↗(On Diff #98607)

"being" can be dropped. (You are using passive present progressive tense, probably not what you intended)

at (article missing?) load/store

lib/Analysis/ScopInfo.cpp
1037 ↗(On Diff #98607)

This is not too important. Readers should be able to notice that "FAD" stands for "FortranArrayDescriptor" if documented in some comments.

test/ScopDetect/fortran_array_param_nonmalloc_nonvectored.ll
1–5 ↗(On Diff #98607)

Please remove all switches that are not needed for this test case.

Also add %loadPolly.

I think I already remarked this for you previous commit.

7–8 ↗(On Diff #98607)

Please remove

39 ↗(On Diff #98607)

Remove #0, metadata is missing for it. Also remove the "nounwind uwtable" which doesn't apply without the metadata.

bollu updated this revision to Diff 98759.May 12 2017, 7:11 AM
  • [NFC wrt patch] incorporate changes suggested by @Meinersbur
bollu added inline comments.May 12 2017, 7:13 AM
lib/Analysis/ScopBuilder.cpp
315 ↗(On Diff #98607)

I suppose, but I want to clarify what *kind* of pattern I am matching against as well. That makes it quite long, unfortunately.

bollu updated this revision to Diff 98760.May 12 2017, 7:17 AM
  • [NFC wrt patch] add newline to test case, rewrord and rename isFortranArray
Meinersbur accepted this revision.May 12 2017, 12:11 PM

Test case look sufficiently small. Do you still intent do reduce it?

Except a few nits, LGTM.

include/polly/ScopBuilder.h
111 ↗(On Diff #98760)

match

125 ↗(On Diff #98760)

load/store (or LoadInst/StoreInst), accesses

lib/Analysis/ScopBuilder.cpp
126 ↗(On Diff #98760)

Nit: dot at the end of sentence.

142 ↗(On Diff #98760)

array

146 ↗(On Diff #98760)

Empty @returns

154 ↗(On Diff #98760)

Explicit comparison with nullptr is not necessay:

assert(Ty && ...)
155 ↗(On Diff #98760)

Nit: auto *StructArrTy or StructType *StructArrTy

test/ScopInfo/fortran_array_param_nonmalloc_nonvectored.ll
1 ↗(On Diff #98760)

-S has no effect with -analyze

2 ↗(On Diff #98760)

Please remove unnecessary flags. At least -polly-process-unprofitable is redundant, as already contained in %loadPolly

24–25 ↗(On Diff #98760)

Lines can be removed.

This revision is now accepted and ready to land.May 12 2017, 12:11 PM
bollu updated this revision to Diff 98879.May 13 2017, 4:13 AM
  • [NFC wrt patch] fix style, naming issues
bollu updated this revision to Diff 98880.May 13 2017, 4:15 AM
  • [NFC wrt patch remove redundant -polly-process-unprofitable
bollu updated this revision to Diff 98881.May 13 2017, 4:17 AM
  • [NFC wrt patch] arrray -> array
bollu added a comment.May 13 2017, 4:28 AM

@Meinersbur: I've addressed your comments, could you take a look?

bollu closed this revision.May 15 2017, 1:37 AM
This comment was removed by bollu.
bollu reopened this revision.May 15 2017, 1:39 AM

Closed by accident, meant to close D33174

This revision is now accepted and ready to land.May 15 2017, 1:39 AM
This revision was automatically updated to reflect the committed changes.