At the moment, there is an issue with utilising an OpenMP Dialect TargetOp inside of an MLIR → LLVM-IR test file triggering an error when using the following command: “mlir-translate -mlir-to-llvmir -split-input-file”.
This is due to the fact that when lowering the TargetOp to LLVM-IR we utilise a shared piece of infrastructure with Clang that generates some unique information we use to name the target kernels uniquely but consistently for host and device from the file, however, -split-input-file appears to modify the filename stored in FileLineColLoc (which is how we retrieve the filename currently) with some additional information stating that it’s a split and the location in the file the split resides. This unfortunately means if we use the unaltered filename from FileLineColLoc in these scenarios we end up with an error due to the incorrect filename.
I currently have a Phabricator patch up for this with a naïve fix: ⚙ D153781 [MLIR][OpenMP] Use getTargetEntryUniqueInfo from IRBuilder and fix an issue when using target op with --split-input-file option which essentially just trims the extra information from the filename. However, this obviously has some issues, one being that if the extra appended information ever changes this code no longer works, so it doesn’t seem like the preferred way to do this.
So I was wondering if there was an alternative more robust way of doing this, perhaps there is another way to retrieve the original filename?
I have a few ideas inline in the patch I linked, such as:
• Passing down an alternative filename as an attribute from the frontend similar to what I believe was proposed previously and rejected. Seems the easiest and less prone to breakage due to a change in MLIR infrastructure and something similar is done for accessing the host LLVM-IR file on the device pass currently.
• Calculate the unique information differently, I’m not really sure how we’d go about doing this if we can’t access the contents of the file, perhaps use the loaded in MLIR module data in some way to unique the information. I’m also not a huge fan of doing this differently from how Clang currently does it, at least while we’re still glue-ing things together for the OpenMP dialects GPU flow.
• Use this change for the time being on the expectation that the MLIR infrastructure doesn’t change this segment
But perhaps someone has alternative ideas that I’ve not thought of (very likely, still a lot to learn) or perhaps my thinking on the issue is incorrect!
1 Like
I am not sure whether this idea is doable, but we could also think about creating combined Location objects for the MLIR modules created for split buffers, with separate information about the original source file and the split. Rather than creating a FileLineColLoc with split information encoded in the file name, we could create a FusedLoc or another kind of structured Location object allowing the storage of an unmodified FileLineColLoc together with some split location information or tag.
This way I think it should be relatively simple to get the original file name, but there may be some other implications I’m not aware of. I’m not very familiar with the process by which Locations get created initially for split buffers, so it may not even be easily done. And it may also not be the intended use of FusedLoc. Just another approach that could be worth considering.
1 Like
At one point I was playing with it to try and see if I could have name resolution on the main buffer while parsing a secondary one inside the SourceManager, that would have resulted in only parsing a section while having the original line info. But I ran into an issue with that and had to context switch. But basically the only reason we have the split marker text is to enable parsing on a sub view of the memory buffer but with traceability back to main. If something like that could be done, then it would resolve the rest and would be nicer for IDEs too.
1 Like
I think that the root cause of the bug is missed location attribute in MLIR module. I think that all MLIR tools should preserve link between MLIR and the source code.
I was able to dump location attribute for test.f95 when I use bbc tool ( bbc -fopenmp test.f95 --mlir-print-debuginfo ):
subroutine test
implicit none
integer :: i
!$omp target map(from:i)
i = 10
!$omp end target
end subroutine
The output file contains FIR dialect + OpenMP dialect. Each MLIR operation contains information about the corresponding source code (for example: omp.terminator loc("test.f95:4:9") ).
Unfortunately I was not able to preserve source code location when I used fir-opt to generate LLVMIR dialect from FIR dialect. That’s why I prepared an example MLIR module with LLVMIR + OpenMP dialect by hand:
module {
llvm.func @test() {
omp.target {
omp.terminator loc("test.f95":1:1)
} loc("test.f95":3:1)
llvm.return loc("test.f95":2:1)
} loc("test.f95":1:1)
} loc("test.f95":0:0)
and I was able to run mlir-translate --mlir-to-llvmir --split-input-file command without any error. If loc attributes are missed, then I got Unable to get unique ID for file error for mlir-translate.
Having said that my recommendations are as follows:
- Modify
flang-new -save-temps options so that it dumps also location attributes. We can use bbc --mlir-print-debuginfoas the reference.
- Modify
fir-opt tool so that it preserves location attributes.
1 Like
Thank you everyone for your very helpful suggestions, I’m going to have a deeper look into the directions suggested and see if I can come up with anything. It might take me a week or so till I can have a look into it further, as there’s another piece of work I’m looking into at the moment as well!
But expect to see the revival of the thread at some point with hopefully a more reasonable solution than the one in my first Phabircator patch or failing that some further questions. However, if you have further suggestions or thoughts in the meantime please do add them, I appreciate the help immensely.
1 Like
Let me add one more remark to my previous post:
If location attribute for omp.target operation is required for lowering into LLVM IR, then we should modify mlir-translate tool to provide a meaningful feedback for the user instead of trying to figure out the original source file.
IMO Unable to get unique ID for file error does not provide the meaningful feedback.
An example of mlir-translate modification which provides meaningful feedback for the user:
Invalid input code:
omp.target {
omp.terminator
} //invalid mlir - no location provided
Proposed output for mlir-translate --mlir-to-llvmir --split-input-file target_no_loc.mlir:
error: omp.target operation requires location attribute for lowering MLIR code to LLVM IR
In this case the “Unable to get unique ID for file” error comes from OpenMPToLLVMIRTranslation.cpp during the lowering, so it could be possible to just give better error information and put the onus on the test writer/user to create an explicit loc. It might require extending the OMPIRBuilder function a little to give an optional error callback function.
However, I am not sure it can be quite as exact as stating it requires a location attribute, perhaps just a suggestion that it might be the cause, primarily as we can’t tell it’s the exact reason it’s failing at that point, unless we introspect the file location to see if it has a filename generated by the split tool, and if we do that it’s essentially the same as the Phabricator patch I have up at the moment (just swapping the trimming of the extra information to get it to compile for a more explanative error message).
Perhaps you could alter the segment that initially generates the location information when you invoke mlir-translate to emit the error based on if explicit location information is specified or not already when in conjunction with OpenMP Ops that require explicit location information, as it seems to not overwrite the existing specified location with the auto-generated split location. But I’m unsure how much push-back that’d get from the wider MLIR community, perhaps it’s reasonable though.
If putting the onus on the user is something we’d rather do, the code could perhaps be left as is just now, or I could extend the existing OpenMP → LLVM-IR infrastructure to suggest the user makes sure they’ve defined the location information explicitly (might be a bit of a verbose error message though) or look into seeing how feasible altering mlir-translates infrastructure to support specific error emission for these ops when no location is provided.