This is an archive of the discontinued LLVM Phabricator instance.

[Flang] Fixes for XArrayCoorOp
ClosedPublic

Authored by kiranchandramohan on Jun 16 2022, 4:07 PM.

Details

Summary

The upstreamed code was not incrementing the sliceOffset in multiples
of 3. This issue is fixed by using Offsets and incrementing by 3 during
every iteration.
In the conversion pattern, we were comparing the definingOp of an
operand with an FIR::UndefOp. Use LLVM::UndefOp for conversion.

Diff Detail

Event Timeline

Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJun 16 2022, 4:07 PM
Herald added a subscriber: mehdi_amini. · View Herald Transcript
kiranchandramohan requested review of this revision.Jun 16 2022, 4:07 PM
clementval accepted this revision.EditedJun 16 2022, 11:23 PM

This increment was upstreamed correctly but it was removed by https://reviews.llvm.org/D125967.

Otherwise LGTM.

This revision is now accepted and ready to land.Jun 16 2022, 11:23 PM

This increment was upstreamed correctly but it was removed by https://reviews.llvm.org/D125967.

Otherwise LGTM.

Ahh, I see. Now I understand why that code was in the original way. I guess it was to check with the fir::UndefOp.

flang/lib/Optimizer/CodeGen/CodeGen.cpp
2107

I have opened a discussion in https://discourse.llvm.org/t/order-of-conversion-of-operations/63259 for checking whether comparing with LLVM::UndefOp is the correct way. It seems it is not entirely safe to do that comparison.

May be we should do the comparison on the original Operands, and create step with the converted operands. But I am getting a crash with the getDefiningOp call in some cases with the original Operand.

Use the originalOperands to check whether it is an fir::UndefOp
instead of using the newOperands and checking whether it is an
LLVM::UndefOp.

Leporacanthicus accepted this revision.Jun 17 2022, 10:06 AM

LGTM.
Tested locally, works for me!

This revision was automatically updated to reflect the committed changes.

Ahh, I see. Now I understand why that code was in the original way.

+1. Sorry for the previous incorrect remove. I go through the conversion of fir::cg::XArrayCoorOp and have several nit comments. In addition, operands[0] is commonly used, should it be assigned to one variable coorBaseAddr/coorBase similar to the conversion of fir::CoordinateOp so to make it more readable? @kiranchandramohan Can you commit one NFC for these changes if they look OK to you?

flang/lib/Optimizer/CodeGen/CodeGen.cpp
2125
2145
2147
2193
2202
2206
2214

Ahh, I see. Now I understand why that code was in the original way.

+1. Sorry for the previous incorrect remove. I go through the conversion of fir::cg::XArrayCoorOp and have several nit comments. In addition, operands[0] is commonly used, should it be assigned to one variable coorBaseAddr/coorBase similar to the conversion of fir::CoordinateOp so to make it more readable? @kiranchandramohan Can you commit one NFC for these changes if they look OK to you?

Thanks @peixin. I believe D129071 addresses some of these. I will make any remaining changes post completion of upstreaming.