A logic incompleteness may lead MemorySSA to be too conservative in its results. Specifically,
when dealing with a call of kind call i32 bitcast (i1 (i1)* @test to i32 (i32)*)(i32 %1),
where function call test is declared with readonly attribute, the bitcast is not wrapped,
obscuring function attributes. Hence, some methods of CallBase (e.g., doesNotReadMemory)
could provide incomplete results. This issue was addressed with improved checks.
Details
Diff Detail
Unit Tests
Event Timeline
llvm/lib/IR/Instructions.cpp | ||
---|---|---|
352–359 | You should be able to drop the getCalledFunction() part, as it is subsumed by the newly added code. |
I'm slightly worried about the inconsistency but I am convincing myself slowly this is fine to do. I also imagine a single bitcast is the most important case to catch. The tests for MSSA is a bit obscure but I guess there is sufficient test coverage across the 4 tests.
The Attributor tests need to be updated properly though. I left comments.
llvm/test/Transforms/Attributor/IPConstantProp/arg-count-mismatch.ll | ||
---|---|---|
1 | Don't change the options. | |
135 | Run the update script so these things don't go away. |
llvm/lib/IR/Instructions.cpp | ||
---|---|---|
355–356 | Style suggestion, OK to ignore. I find it easier to read (less casts). |
I'm okay with this for the case of function attributes in particular, because those don't really depend on ABI considerations. I will say though that you're brushing really close to UB here, and I may push back against extending other places in this direction. Like, say you have a void(i128) and call it as void(i64, i64) knowing that under the particular ABI used those two i64s will assemble into an i128 -- is that a legal call? In what manner would a function param attribute correlate with call arguments there?
I second this.
Recently I (had to) start to fix all the places in the Attributor that are not aware that call site and callee might mismatch wrt. parameters (due to casts). It's a mess even w/o attributes.
For functions we should be OK because there is no ambiguity what the "function" is.
Haven't really thought about it, yet, like @jdoerfert noted, there should exist no ambiguity as far as the function is concerned.
I suspect methods CallBase::getIntrinsicID and CallBase::paramHasAttr might be changed as well in the future.