[PATCH] D99967: [Flang] Changes to mangling code

[Kiran Chandramohan via Phabricator via llvm-commits]

kiranchandramohan added a comment.

In D99967#2688121 <https://reviews.llvm.org/D99967#2688121>, @dblaikie wrote:

> In D99967#2688105 <https://reviews.llvm.org/D99967#2688105>, @kiranchandramohan wrote:
>
>>>>> Though the description mentions that the functions are all tested - yet it looks like this patch adds a new function/overload without any callers, so how is it tested?
>>>>
>>>> Apologies for the misunderstanding here. What i meant is that the function that is ultimately called (fir::NameUniquer::doType) is already tested and this is a wrapper around that function. I will check today if I can add a unit  
>>>>  test for this function (assuming that is the recommendation here).
>>>
>>> Hmm - well, then there's the question of: What's the purpose of the new function? We don't usually add new code without usage (& then test the new code via that usage). But there are some times when it's suitable to implement new code with nothing but a unit test for coverage - usually only if that code is intended for API usage by clients outside the LLVM project.
>>>
>>> What's the purpose/intended usage of this new code?
>>
>> Names have to be unique at the FIR (MLIR IR for Flang) layer. So the usage of name mangling is when we lower from the parse-tree of Flang to FIR. The bridge code which does this lowering has not yet landed in llvm-project/flang. The approach for upstreaming this portion is to bring in all the support functions and classes and then introduce the bridge code and show the lowering and testing for each Fortran construct.
>
> That presents a challenge for testing - usually we'd implement partial support for a feature more end-to-end (ie: make it "work" but without all the functionality) then as functionality is added incrementally, so is corresponding testing.
>
> But adding lots of implementation code that's basically "dead" until the final patches wire it up makes it hard to tell which parts are tested - it'll need to be significantly unit tested & be a bit out of sync with the way the rest of LLVM is authored and tested.

I am not disagreeing with the two points that you suggest. The partial end-to-end support was suggested but did not get the flang community approval. I believe the difficulty included finding small patches which implement end to end functionality that can be upstreamed. For e.g. when i tried to upstream the parse-tree to FIR lowering (https://reviews.llvm.org/D79731) of an empty subroutine in May last year it was a huge patch and there was not much support for it.

>> (Note that at the FIR layer and lowering to LLVM IR the agreed approach is to upstream pass by pass).
>
> I'm not especially familiar with MLIR or Flang to properly parse this statement - could you rephrase/add more detail?

I was just highlighting the difference in approach of upstreaming the parse tree -> MLIR lowering and the MLIR -> MLIR transformations/MLIR -> LLVM IR lowering. 
There is a lot of support code required for the former (parse tree -> MLIR) due to the big difference in representation of the parse-tree and MLIR. And all this support code is written from scratch.
In the case of MLIR transformations or lowering to LLVM IR a lot of support code already exists in llvm-project and hence for these cases we can upstream in the traditional way and it is the agreed approach for upstreaming. This would be like upstreaming the lowering of each MLIR operation or upstreaming each pass which does a transformation on MLIR.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99967/new/

https://reviews.llvm.org/D99967