This is an archive of the discontinued LLVM Phabricator instance.

Fortran/gfortran/regression/gomp/DisabledFilesHLFIR.cmake
9	It might be preferable to have something like this to indicate where `SKIPPED_FILES` has been defined instead of just omitting it altogether. While omitting it will work, but being explicit - if a bit verbose - might make maintenance easier. set(SKIPPED_FILES "")

This revision now requires changes to proceed.Aug 1 2023, 1:52 PM

tblah added a subscriber: vzakhari.Aug 2 2023, 1:40 AM

tblah added inline comments.

Fortran/gfortran/regression/gomp/DisabledFilesHLFIR.cmake
9	@vzakhari asked on my last patch that I reset the variables once in CMakeLists.txt and not mention them in every included file. https://reviews.llvm.org/D156130#4532372 But if you would rather keep the set(VARIABLE "") then I don't mind changing

tarunprabhu added inline comments.Aug 2 2023, 7:48 AM

Fortran/gfortran/regression/gomp/DisabledFilesHLFIR.cmake
9	I'm not fan of all those empty declarations in the files either and agree that it would be better to declare them just once in that macro. However, I would prefer to leave them be for now. There may be more churn on that front as development continues and having those be explicit seems preferable to me. I would prefer to get rid of the unnecessary files when things are more stable and there is less likelihood of having to reintroduce those lists because of some new feature development. If @vzakhari has strong views the other way, I'm willing to defer to those.

vzakhari added inline comments.Aug 2 2023, 8:25 AM

Fortran/gfortran/regression/gomp/DisabledFilesHLFIR.cmake
9	It sounds like trying to use code redundancy for documentation. If this is the reason you want to have the empty `set` commands, may I suggest just putting a link to the documentation describing all the support variables into each feature cmake file? This way if we need to change the set of supported variables, we do not need to go through all the dummy `set` commands and renamed/delete them.

@tblah On x86_64, with HLFIR enabled, regression/achar_4.f90 fails with the error 'fir.convert' op invalid type conversion. I don't think it is related to this patch, but I am not sure. I assume that you are on AArch64 and don't see this?

Fortran/gfortran/regression/gomp/DisabledFilesHLFIR.cmake
9	@vzakhari That's a fair point. I'm working on updating the list of top-level disabled tests. I'll consider performing some sort of cleanup then.

In D156820#4554865, @tarunprabhu wrote:

@tblah On x86_64, with HLFIR enabled, regression/achar_4.f90 fails with the error 'fir.convert' op invalid type conversion. I don't think it is related to this patch, but I am not sure. I assume that you are on AArch64 and don't see this?

I can't reproduce that on my aarch64 machine, although this test doesn't look like it should change much between architectures. I tested on llvm-project commit eb6987027e0504adcdc319f080a9ea48aab2a72a both with and without optimization flags.

But if it doesn't pass for you, I think we should disable it for now (it should be removed from Fortran/gfortran/regression/EnabledFilesHLFIR.cmake). But I think it should be done separately to this patch as it is unrelated. Let me know if you would like me to put a patch up for this.

I just checked again on 29dec352124a5fc1dcf03a640c2621496651ee90 and it still breaks.

A separate patch will be helpful. Thanks!

This revision is now accepted and ready to land.Aug 3 2023, 7:01 AM

tblah mentioned this in D157084: [Fortran][hlfir] Disable regression/achar_4.Aug 4 2023, 4:26 AM

Closed by commit rT426e6cb0d32d: [Fortran][hlfir] update openmp denylists with new fixes (authored by tblah). · Explain WhyAug 4 2023, 4:27 AM

This revision was automatically updated to reflect the committed changes.

tblah added a commit: rT426e6cb0d32d: [Fortran][hlfir] update openmp denylists with new fixes.

In D156820#4557523, @tarunprabhu wrote:

I just checked again on 29dec352124a5fc1dcf03a640c2621496651ee90 and it still breaks.

A separate patch will be helpful. Thanks!

https://reviews.llvm.org/D157084

tblah mentioned this in rT6421a2df07e7: [Fortran][hlfir] Disable regression/achar_4.Aug 4 2023, 6:24 AM

Revision Contents

Path

Size

Fortran/

gfortran/

regression/

gomp/

DisabledFilesHLFIR.cmake

9 lines

appendix-a/

DisabledFilesHLFIR.cmake

21 lines

Diff 547164

Fortran/gfortran/regression/gomp/DisabledFilesHLFIR.cmake

	#===------------------------------------------------------------------------===#			#===------------------------------------------------------------------------===#
	#			#
	# Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.			# Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
	# See https://llvm.org/LICENSE.txt for license information.			# See https://llvm.org/LICENSE.txt for license information.
	# SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception			# SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
	#			#
	#===------------------------------------------------------------------------===#			#===------------------------------------------------------------------------===#

	file(GLOB SKIPPED_FILES CONFIGURE_DEPENDS
	tarunprabhuUnsubmitted Not Done Reply Inline Actions It might be preferable to have something like this to indicate where `SKIPPED_FILES` has been defined instead of just omitting it altogether. While omitting it will work, but being explicit - if a bit verbose - might make maintenance easier. set(SKIPPED_FILES "") tarunprabhu: It might be preferable to have something like this to indicate where `SKIPPED_FILES` has been…
	tblahAuthorUnsubmitted Not Done Reply Inline Actions @vzakhari asked on my last patch that I reset the variables once in CMakeLists.txt and not mention them in every included file. https://reviews.llvm.org/D156130#4532372 But if you would rather keep the set(VARIABLE "") then I don't mind changing tblah: @vzakhari asked on my last patch that I reset the variables once in CMakeLists.txt and not…
	tarunprabhuUnsubmitted Not Done Reply Inline Actions I'm not fan of all those empty declarations in the files either and agree that it would be better to declare them just once in that macro. However, I would prefer to leave them be for now. There may be more churn on that front as development continues and having those be explicit seems preferable to me. I would prefer to get rid of the unnecessary files when things are more stable and there is less likelihood of having to reintroduce those lists because of some new feature development. If @vzakhari has strong views the other way, I'm willing to defer to those. tarunprabhu: I'm not fan of all those empty declarations in the files either and agree that it would be…
	vzakhariUnsubmitted Not Done Reply Inline Actions It sounds like trying to use code redundancy for documentation. If this is the reason you want to have the empty `set` commands, may I suggest just putting a link to the documentation describing all the support variables into each feature cmake file? This way if we need to change the set of supported variables, we do not need to go through all the dummy `set` commands and renamed/delete them. vzakhari: It sounds like trying to use code redundancy for documentation. If this is the reason you want…
	tarunprabhuUnsubmitted Not Done Reply Inline Actions @vzakhari That's a fair point. I'm working on updating the list of top-level disabled tests. I'll consider performing some sort of cleanup then. tarunprabhu: @vzakhari That's a fair point. I'm working on updating the list of top-level disabled tests.
	# error: operand#0 does not dominate this use
	do-1.f90
	nontemporal-1.f90
	pr59488-1.f90
	pr69183.f90
	pr78298.f90
	)

Fortran/gfortran/regression/gomp/appendix-a/DisabledFilesHLFIR.cmake

	#===------------------------------------------------------------------------===#			#===------------------------------------------------------------------------===#
	#			#
	# Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.			# Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
	# See https://llvm.org/LICENSE.txt for license information.			# See https://llvm.org/LICENSE.txt for license information.
	# SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception			# SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
	#			#
	#===------------------------------------------------------------------------===#			#===------------------------------------------------------------------------===#

	file(GLOB UNIMPLEMENTED_FILES CONFIGURE_DEPENDS
	# (hlfir+omp): unimplemented: Common block in privatization clause
	a.23.4.f90
	)

	file(GLOB SKIPPED_FILES CONFIGURE_DEPENDS			file(GLOB SKIPPED_FILES CONFIGURE_DEPENDS
	#			#
	# These tests fail when they should pass.			# These tests fail when they should pass.
	#			#

	# (hlfir+omp): operand#0 does not dominate this use
	a.1.1.f90
	a.6.1.f90
	a.8.1.f90
	a.12.1.f90
	a.21.2.f90
	a.21.3.f90
	a.26.2.f90
	a.30.1.f90
	a.31.1.f90
	a.31.2.f90
	a.34.1.f90
	a.34.2.f90
	a.35.2.f90
	a.37.1.f90

	# (hlfir+omp): builder.create<DeclareOp>() assertion failure			# (hlfir+omp): builder.create<DeclareOp>() assertion failure
	a.17.1.f90			a.17.1.f90
	a.17.2.f90			a.17.2.f90
	a.17.3.f90			a.17.3.f90

	# (hlfir+omp): Fortran::lower::genThreadprivateOp assertion failure			# (hlfir+omp): Fortran::lower::genThreadprivateOp assertion failure
	a.22.1.f90			a.22.1.f90

	# (hlfir+omp): "the thread private operation not created" assertion failure			# (hlfir+omp): "the thread private operation not created" assertion failure
	a.32.1.f90			a.32.1.f90
	)			)

This is an archive of the discontinued LLVM Phabricator instance.

[Fortran][hlfir] update openmp denylists with new fixesClosedPublic

Details

Diff Detail

Event Timeline

Revision Contents

Diff 547164

Fortran/gfortran/regression/gomp/DisabledFilesHLFIR.cmake

Fortran/gfortran/regression/gomp/appendix-a/DisabledFilesHLFIR.cmake

[Fortran][hlfir] update openmp denylists with new fixes
ClosedPublic