Page MenuHomePhabricator

[flang]Fix incorrect array type transformation
ClosedPublic

Authored by Leporacanthicus on Jul 6 2022, 5:54 AM.

Details

Summary

When an array is defined with "unknown" size, such as fir.array<2x?x5xi32>,
it should be converted to llvm.array<10 x i32>. The code so far has
been converting it to llvm.ptr<i32>.

Using a different function to check the if there starting are constant
dimensions, rather than if ALL dimensions are constant, it now produces
the correct array form.

Some tests has been updated, so they are now checking the new behaviour
rather than the old behaviour - so there's no need to add further tests
for this particular scenario.

This was originally found when compiling Spec 17 code, where an assert
in a GepOP was hit. That is bug #56141, which this change fixes.

Diff Detail

Event Timeline

Leporacanthicus created this revision.Jul 6 2022, 5:54 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJul 6 2022, 5:54 AM
Herald added a subscriber: mehdi_amini. · View Herald Transcript
Leporacanthicus requested review of this revision.Jul 6 2022, 5:54 AM
flang/test/Fir/alloc.fir
282

Would this end up over-allocating? There is already a multiplication by 60 (= 4 * 3 *5) for prod1. That seems to have included the 4x portion.

Thanks for fixing the issue

flang/test/Fir/alloc.fir
282

I think it would, yes. @Leporacanthicus, you might need to also update genAllocationScaleSize here: https://github.com/llvm/llvm-project/blob/b094c737cd85b8bf979c3cc998853b03a20d5c59/flang/lib/Optimizer/CodeGen/CodeGen.cpp#L317.

It should probably translate the type to LLVM, and only use the constant extents from the FIR sequence ty[e dimensions that where not kept in the LLVM type.

I think this will remove all usages of hasConstantInterior that was non trivial, so you could delete it then.

flang/test/Fir/alloc.fir
282

Thanks for the hint. I've managed to fix one variety, but in the process broken another.

It's not hard to fix, but I think I've got to split the calculation into two parts, one for dynchar arrays, and one for the non-dynchar variety. Or something like that.

I will continue Tuesday (off work on Monday).

Updated genAllocationScaleSize and removed now unused hasConstantInterior.
Updated tests to match new allocation scale size (adjusted for the type size being different).

Thanks for cleaning-up the suggestion. This fix helps straighten things out ! A few more suggestions.

flang/lib/Optimizer/CodeGen/CodeGen.cpp
346

Shouldn't that be an else if to avoid counting the extents twice if there was a shape operand ?

Anyway, I am thinking that given the intent of getConstantRows, it should return zero if the sequence element type has a dynamic size. That way I think you could simply have a single if(seqTy && seqTy.getConstantRows() != seqTy.getShape().size()) {...} case.

You would need to update the current hasConstantShape() to not use it anymore because there are usages in lowering that do not care about the element having dynamic size or not (it can become a simple loop on the extents checking if they are constants).

You could rename the current version of hasConstantShape() into hasDynamicSize() for its uses here.

flang/lib/Optimizer/CodeGen/TypeConverter.h
321

With the suggested change, the two ifs could become a single if (!seqTy.hasDynamicSize()) (the current hasConstantShape(), renamed)

349

If getConstantRows is updated as suggested, I think you could also remove that special case about characters here.

359

You would have to use seq.hasDynamicSize() here.

Updates based on review comments:

  • Rename hasConstantShape to inverse and now called hasDynamicSize
  • Return 0 on getConstantRows for dynamicLength character types
  • Simplify code as review commetns suggested
Leporacanthicus marked 3 inline comments as done.Mon, Jul 25, 5:20 AM

Other than my two nits about naming and the fact that fir::hasDynamicType is more generic than characterWithDynamicLen for the future, looks great, thanks !

flang/include/flang/Optimizer/Dialect/FIRTypes.td
463

I am being a bit nitpicky with the name here, but I think this one should be hasDynamicExtents because it does not take into consideration the element type, and I would expect a type containing a fir.char<?> to reply false to hasDynamicSize().

flang/lib/Optimizer/Dialect/FIRType.cpp
786

fir::hasDynamicType is more generic than characterWithDynamicLen, it will account for the future parametrized derived types when they get there.

Update the newly added function to hasDynamicExtents instead of the
already used hasDynamicSize.

Change the "is variable length" to use hasDynamicSize (the original
function) instead of restricting it to characterWithDynamicLen.

Leporacanthicus marked 2 inline comments as done.Thu, Jul 28, 6:33 AM
jeanPerier accepted this revision.Thu, Jul 28, 6:35 AM

Thanks @Leporacanthicus, that looks good to me.

This revision is now accepted and ready to land.Thu, Jul 28, 6:35 AM
This revision was automatically updated to reflect the committed changes.