[PATCH] D95449: [flang] Fix problems with constant arrays with lower bounds that are not 1
Peter Klausler via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Jan 27 09:02:30 PST 2021
klausler added a comment.
In D95449#2524183 <https://reviews.llvm.org/D95449#2524183>, @PeteSteinfeld wrote:
> In D95449#2523153 <https://reviews.llvm.org/D95449#2523153>, @klausler wrote:
>
>> Can you fix the problem by just calling "x.set_lbounds(GetLowerBounds())" on the folded result array and leaving ScalarConstantExpander alone?
>
> The expression that needs to have its lower bounds modified is of type Expr<SomeType>. But the set_lbounds() method is defined for the types Constant<T>, ConstantBase<T>, and ConstantBounds<T>. So to use set_lbounds(), I need to walk down through the layers of Expr<> objects to get to an object of type Constant<T> and then call set_lbounds() on it.
>
> So I could create a class to do this, but that class would be very similar to the exiting class that I've renamed as ArrayConstantMaker. Potential differences would be that it might not require a method that takes Parenthese<T> as an arugment (I'm not sure about this), the call to Reshape() would not be required, and the new class would need fewer constructors.
>
> Thus, it seems to me that it's better to use a single class to create constant arrays with new lower bounds that handles both scalars and arrays.
>
> Let me know if my analysis is wrong, or if you agree with my analysis but not me conclusion.
I think I'd rather have two specifically purposed facilities for scalar expansion and for setting lower bounds than one mash-up of those two. If you must combine them, make sure that there's a CHECK that ensures that the original array is either scalar or has exactly the same shape.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D95449/new/
https://reviews.llvm.org/D95449
More information about the llvm-commits
mailing list