[flang-commits] [flang] [Flang][OpenMP][Driver][Test] Fix the omp-driver-offload-test commands (PR #66926)

[via flang-commits]

agozillon wrote:


> I don't want to come across as picky, but changing `not %flang` to `%flang` is significant - it basically means that in one case the end-user will see their compiler failing and in the other succeeding.

Not picky at all, always happy to have questions/attempt to clarify things! 

The `not %flang` was added as a sweeping set of changes in this commit: https://github.com/llvm/llvm-project/commit/f39c399d9d15efe8309d8aa3d0ecf62205e6c474 as what appears to be a band-aid fix, prior to it they weren't existent and they appear to have been added under the presumption that certain tests will need fixed based on the changes to -### returning error/success codes correctly (e.g. the test improvement list in the commit message, albeit there is none relevant to this test in particular). The tests passed fine all the time previously because they didn't ever fail, now they will fail on certain occasions (whenever they have no arch specified and no library present). 

> > This was because if certain environment variables are set for AMD devices some of the commands succeed happily and the not check causes the test to fail.
> 
> IIUC, this test does not depend on any env variables and it should succeed regardless of the env variables? If it does depend on env variables then that should be documented. Is it? Similar note on various libraries being present or not.

The test itself doesn't depend on any environment variables (or at least I didn't write the initial version with that knowledge/intent), but the success or failure of the commands as they are currently written depends on having rocm's library folder in your path. Defining the architecture and defining the tests around this should avoid this behavior as we're no longer dependent on the rocm library for picking up the native architecture.

> As per PR title:
> 
> > [Flang][OpenMP][Driver][Test] Fix the omp-driver-offload-test commands
> 
> This test works for me fine and from my perspective it is being "refined" rather than "fixed". Perhaps there's an issue that affects some folks and this change resolves that, but from your description it sounds like a "downstream" issue? 

I'm happy with whatever classification we want to give it a refine or fix! I chose fix as the commands are failing currently, which wasn't the intention of them when they were originally added. I don't think it's a downstream issue, I believe it's an issue for anyone developing for AMD GPU in conjunction with a Flang build I think (this test fails in this manner on an unmodified llvm project), the community just happens to be quite small at the moment as it's in development (and it's been noted by @jsjodin and @TIFitis to fail arbitrarily as well, I just happened to stumble across what I believe is the reason why while fixing an unrelated patch).

> Are there any public buildbots affected by this?

There are currently no public buildbots that run Flang+AMDGPU+OpenMP tests at the same time as far as I am aware, and this particular test is tied to Flang, so it's unfortunately fallen under the radar for a little while I think . the closest is this buildbot which tests AMDGPU + OpenMP with a focus on the runtime, but it doesn't have a flang build: https://lab.llvm.org/buildbot/#/builders/193/builds/39058/steps/6/logs/stdio (feels like a bit of a gap as we're beginning to add Flang+OpenMP runtime tests and I don't know who to discuss with about perhaps adding a Flang build for this to get test coverage for these, but might not be worth the extra build time at the moment). 

> "fix" hints to me that there's a bug and in such cases I feel that I should understand what the issue is before I approve a patch. In this case I don't (*), but am happy to rely on your expertise and the expertise of other reviewers. The whole discussion seems to assume a good deal of background on OpenMP offloading for AMD GPUs, which I don't have.
> 
> (*) With the exception of the `-o` changes.

That's understandable. Perhaps it's not necessarily a bug, just a not so future proof test originally written by me I believe so the test is in need of an adjustment. So perhaps fix is a bit strong, I don't mind what wording we use though, so feel free to suggest any changes to the commit message and title you wish!  


https://github.com/llvm/llvm-project/pull/66926