Adds function attributes to index: ReadNone, ReadOnly, NoRecurse, NoAlias. This attributes will be used for future ThinLTO optimizations that will propagate function attributes across modules.
Details
Diff Detail
Event Timeline
Currently, two tests are failing: ThinLTO/X86/autoupgrade.ll and ThinLTO/X86/drop-debug-info.ll, since they have blobs with old versions of the bitcode. Should I update those as well?
include/llvm/IR/ModuleSummaryIndex.h | ||
---|---|---|
62 | I don't think you're populating these in any way. For now I would leave this out of the patch and focus on the function summary part of it only. |
They're missing the new field I added to the bitcode, so they fail a bounds-check assert.
I'm glad we have these tests then :)
That's a real bug in your code: the bitcode has to be designed so that you can read back older version.
I think you simply need to bump the INDEX_VERSION (in BitcodeWriter.cpp). Looks like you left it at 3, and your reads of the new flags are checking if the version is >=3, but it should presumably be reading the new field at version > 3 with the new version set to 4.
OK, I didn't realize that version number was just used for bookkeeping the bitcode format. That was what I needed to do.
I don't think you're populating these in any way. For now I would leave this out of the patch and focus on the function summary part of it only.
Good point - I forgot to remove the MemoryAccess field. I'll rip it out from the bitcode and the index (and revert those parts of the tests).
Overall looks good, just a few minor things below. Do you have a follow up dependent patch you can send for review that shows how one of these attributes will be utilized? I'd prefer to have this committed when we have a user for at least one of these new attributes.
include/llvm/IR/ModuleSummaryIndex.h | ||
---|---|---|
305 | Needs a comment here as well. Probably just a clone of the one on the structure is fine. | |
lib/Bitcode/Reader/BitcodeReader.cpp | ||
5099–5100 | better to move "numrefs x valueid" to the same line, so that it is formatted consistently (see FS_PERMODULE_PROFILE just below) | |
test/Bitcode/thinlto-function-summary-functionattrs.ll | ||
5 | Can you add a check for noalias as well? |
test/Bitcode/thinlto-function-summary-functionattrs.ll | ||
---|---|---|
5 | Sure - is there some trick to force a function to be marked as noalias? I can't find any examples of noalias functions in any of the tests and this doesn't seem to work: define noalias i32* @i() { %d = alloca i32; ret i32* %d; } |
test/Bitcode/thinlto-function-summary-functionattrs.ll | ||
---|---|---|
5 | I see a few in the tests, here are some samples: In what way does it not work when you try it for your small example? |
lib/Bitcode/Reader/BitcodeReader.cpp | ||
---|---|---|
5117 | I don't think that will work since we have a variable number of reference and call edge records following the flags. We have the number of references (NumRefs) but not the number of calls. That is deduced from the length of the record. |
test/Bitcode/thinlto-function-summary-functionattrs.ll | ||
---|---|---|
5 | The functions (even with the noalias keyword) don't have the NoAlias flag set on them in my example or in the tests. |
test/Bitcode/thinlto-function-summary-functionattrs.ll | ||
---|---|---|
5 | Somehow it is getting propagated. I.e. I took one of these tests (DeadStoreElimination/simple.ll) and did the following $ llvm-as test/Transforms/DeadStoreElimination/simple.ll -o - | llvm-dis -o - | grep test23 I get: Can you check this example? |
test/Bitcode/thinlto-function-summary-functionattrs.ll | ||
---|---|---|
5 | To save you some work, since I (sort of) see what is going on by looking in the debugger: (gdb) p F.dump() ; Function Attrs: nounwind ssp uwtable %x = alloca [2 x i8], align 1 %arrayidx = getelementptr inbounds [2 x i8], [2 x i8]* %x, i64 0, i64 0 store i8 97, i8* %arrayidx, align 1 %arrayidx1 = getelementptr inbounds [2 x i8], [2 x i8]* %x, i64 0, i64 1 store i8 0, i8* %arrayidx1, align 1 %call = call i8* @strdup(i8* %arrayidx) #1 ret i8* %call } Note the function is dumped with "noalias", but doesn't have the NoAlias attribute. However, the return is marked no alias. And looking at the FunctionAttrs.cpp NoAlias propagation (addNoAliasAttrs), it is this (the returnDoesNotAlias) that is being propagated. So perhaps that's what you need to be checking. Probably change the name of the boolean flag in the index accordingly. |
test/Bitcode/thinlto-function-summary-functionattrs.ll | ||
---|---|---|
5 | And just to close the loop on this, looking at AssemblyWriter::printFunction, the ReturnIndex attributes are printed before the function name (which matches where we see "noalias" on functions), whereas the FunctionIndex attributes are printed after the function name (where you see readonly, etc being printed). |
Note the function is dumped with "noalias", but doesn't have the NoAlias attribute. However, the return is marked no alias. And looking at the FunctionAttrs.cpp NoAlias propagation (addNoAliasAttrs), it is this (the returnDoesNotAlias) that is being propagated. So perhaps that's what you need to be checking. Probably change the name of the boolean flag in the index accordingly.
Ah, now it makes sense why the it was still propagating the alias attribute even though that NoAlias flag wasn't set. I'll correct my patch.
lib/Bitcode/Reader/BitcodeReader.cpp | ||
---|---|---|
5117 | Yes you're right. |
LGTM with one last question/nit. Do you have commit access and if not do you want me to submit for you? Do you have follow-on patches that use this about ready to go for review?
test/Bitcode/thinlto-function-summary-functionattrs.ll | ||
---|---|---|
28 | Is malloc call here necessary for test case? |
Yup, I got commit access last week.
Do you have follow-on patches that use this about ready to go for review?
I'm currently working on cleaning up the patch for the GraphTraits for the function summary (planning on submitting that today), then I'll be able to add ReadNone/ReadOnly/NoRecurse propagation (since they all require SCC's). Do you want me to hold off on submitting this patch until one of those are ready to go?
Great!
Do you have follow-on patches that use this about ready to go for review?
I'm currently working on cleaning up the patch for the GraphTraits for the function summary (planning on submitting that today), then I'll be able to add ReadNone/ReadOnly/NoRecurse propagation (since they all require SCC's). Do you want me to hold off on submitting this patch until one of those are ready to go?
Nah, go ahead, let's get this one in.
Thanks!
I don't think you're populating these in any way. For now I would leave this out of the patch and focus on the function summary part of it only.