[PATCH] D18836: Make aliases explicit in the module summary

Teresa Johnson via llvm-commits llvm-commits at lists.llvm.org
Fri Apr 8 09:26:58 PDT 2016


tejohnson added inline comments.

================
Comment at: lib/Bitcode/Writer/BitcodeWriter.cpp:2941
@@ -2933,2 +2940,3 @@
+    // ThinLTO and should be renamed, but if any remains here, just skip them.
     if (!F.hasName())
       continue;
----------------
joker.eph wrote:
> tejohnson wrote:
> > joker.eph wrote:
> > > tejohnson wrote:
> > > > joker.eph wrote:
> > > > > tejohnson wrote:
> > > > > > Saw that you sent a patch to rename them on which this is dependent (will review that tomorrow morn). Assuming that goes in first, perhaps this should stay an assert? Ditto for the continue you added below to the alias loop.
> > > > > That's why I put the assert originally. But then I wasn't sure anymore: should we forbid `llvm-as -module-summary test.ll` when it contains an alias to an anonymous function? 
> > > > > The current implementation will accept it, but the alias will just not be present in the summary.
> > > > Looked at the anon function renaming pass - ok I see the issue is that it is a pass that it won't kick in for llvm-as. I think that should be fixed - similar to the way with D18763 I am computing the Index in llvm-as by invoking the builder directly, I think that the renaming in your new pass should be structured so that it is done in a separate class that can be invoked either from the pass or out of pass. I don't think we want to get a different index when there are anonymous functions that come directly from source through the pass manager vs from LLVM assembly via llvm-as.
> > > > 
> > > > And then this can be turned back into an assert to catch anything unexpected.
> > > I already made it a utility (there is a free function in a public header), because my original plan was to do exactly what you mention above. However I was not super comfortable making llvm-as depends on the Transformations library, or modifying the IR itself...
> > Ah, see that now about the utility function.
> > 
> > I think it should be moved out of Transforms into some place more accessible. 
> > 
> > IMO changing the IR in llvm-as for this purpose is acceptable, but not sure what the general philosophy is.
> I'll wait for Duncan's opinion on this (he's OOO right now).
> 
> We would break round-tripping through `llvm-as | llvm-dis` when summary is enabled.
What does it mean to break round-tripping? Is it required that the output be identical to the input? Because once I add serialization of the index, that will be a difference. 


http://reviews.llvm.org/D18836





More information about the llvm-commits mailing list