Differential D41104
Set the NoRecurse attribute for the dbg intrinsics. Authored by mattd on Dec 11 2017, 9:15 PM.
Details Fixes PR34696 This patch sets the NoRecurse attribute for the debug intrinsics: The purpose of this change is to not have the debugger intrinsics influence code generation. Previously, if a function made a call to the llvm.dbg.value intrinsic, that caller function would not be optimized with the norecurse attribute, even if it was truly not a recursive function.
Diff Detail Event TimelineComment Actions This looks fine, and I imagine we'll need to do this almost all intrinsics. However, I do wonder, is the attribute propagation semantically correct? What we really want to know is that the callee (an intrinsic in this case) doesn't call the caller, no necessarily that it doesn't call itself. void foo(bool) norecurse;
void bar(bool c) {
if (c)
foo(true);
}
void foo(bool c) {
bar(false);
}in this example, the norecurse on foo is semantically correct, but inferring it on bar wouldn't be correct. Comment Actions Thanks for the feedback Hal. I agree, I think many intrinsics can be added/marked-norecurse. But for this change I just wanted to isolate the change to the debugging intrinsics, and keeping with the theme that dbg intrinsics should not influence code generation. Also, you put forward a valid concern, in your example you mention that 'foo' can be marked as norecurse. That is true in this case, given the input values. Perhaps I am wrong, but I think, from what I've seen, the norecurse logic is built on caller-callee relationship and ignores the dataflow. Comment Actions
As you note, it's wrong for functions outside the current module... but it mostly works out at the moment because we don't mark function declarations norecurse. :) (See D14228 for original discussion.)
This isn't what LangRef says, and it would be kind of hard to nail down a definition which actually works like this. I mean, I guess you could try to define it in terms of the callgraph, but that would have unusual implications. (What happens if your program contains a call which is dynamically dead, and incorrectly marked norecurse?) We probably want a new attribute to express the relevant property, which is essentially that most intrinsics don't call any function defined in your program. Comment Actions
Are you suggesting that we abandon this patch in favor of a new patch that introduces a new attribute? Comment Actions I guess...? I don't like proposing new attributes, but I don't see an alternative. (Granted, it could get a little tricky to define precisely what the attribute means given that our runtime functions are written in C.) If you just want to fix the debuginfo issue, you could just check specifically for debuginfo intrinsics in the FunctionAttrs pass; we treat them specially in many passes. Comment Actions
I agree. We shouldn't define this based only on the static call graph. It should be a dynamic property (like, essentially, everything else in order to avoid the dead-code-changing-semantics issue). It seems like we screwed this up a bit because the inference logic in FunctionAttrs won't clearly do the right thing in such cases (and it can't, because norecurse doesn't really have the right semantic information).
That works.
I think that we'd do this in a similar way to how we define our inaccessiblememonly attribute. You'd say that calling the function will not, directly or indirectly, call any function in the module being compiled.
I think that we can easily do better by calling TTI->isLoweredToCall(F), and ignoring everything for which this is false (which will be for the debug intrinsics, and also for a bunch of other things). Comment Actions Thanks for the suggestion @hfinkel, and I agree, using the isLoweredToCall predicate seems the most comprehensive solution; however, I did find it a bit more intrusive than I was expecting. As a side-note, I slapped together a much simpler/less-intrusive patch, that only special-cased the dbg intrinsics and not checking the lowering information, but I feel using the lowering data is more correct. Comment Actions (Was going to let hfinkel review since he'd started on it. efriedma also had said something). Comment Actions I don't see anything wrong with the patch, exactly... but we probably need to fix TargetTransformInfo::isLoweredToCall to correctly handle intrinsics before we merge this. Comment Actions I'm happy to take a look at cleaning up the isLoweredToCall(). Taking a hint from the comment in that routine, I think it makes sense to move that logic to another class, perhaps the base TargetLowering class, after all, this routine is about lowering. I'm not sure what you all had in mind. The solution I am thinking of would make isLoweredToCall a virtual function in TargetLowering. Each target-specific implementation of TargetLowering could override that routine to handle specific instructions that may/may-not be lowered to a call. As an initial pass we could take the routine as it is now and make that the default behavior, which doesn't change functionality, but opens the doors for other targets to make use of this functionality later. Additionally, I think it would be nice to avoid the string matching on instruction names in the body, but that would probably need to be a target-specific solution. Comment Actions Not sure it needs any refactoring? I'm more concerned about the "if (F->isIntrinsic()) return false;" part. Comment Actions Thanks for the response. As you mentioned earlier in this post, what we really need is an attribute or IntrinsicProperty that specifies whether a particular intrinsic becomes a call instruction. It seems the simplest approach, then, would be to add a property that states if an intrinsic is truly a call. If the property is not set, then the intrinsic is not a call, which is probably true for most of the intrinsics. |