Differential D101806
[TargetLowering] Only inspect attributes in the arguments for ArgListEntry Authored by aeubanks on May 3 2021, 6:13 PM.
Details
Parameter attributes are considered part of the function [1], and like Issues can be diagnosed with D103412. [1] https://llvm.org/docs/LangRef.html#parameter-attributes
Diff Detail
Event TimelineComment Actions There are also two other ISel implementations to consider, perhaps they have the same bug: fastisel and globalisel. I think it's worth a quick code search. I think it's also reasonable to add a dedicated test for this. Calling a function with a mismatched prototype is UB, so the compiler is probably entitled to generate arbitrary code (trap), but it should generate something. The example I provided in the other code review is a good starting point.
Comment Actions I think @rnk probably has more context here. Naively, I'd wonder if/why paramHasAttr should be changed to look at the type of the call entirely - but I guess I have to wrap my head around ABI and non-ABI attributes - though I thought sret was an ABI attribute (I thought it specified something about how return values are passed), for instance. Comment Actions I agree, sret is an ABI attribute. :) I think the current paramHasAttr behavior is designed to help optimization passes, not codegen lowering passes. However, I'm not sure how helpful this really is, and we should consider revisiting it. @jdoerfert, in your experience, is this CallBase::paramhasAttr behavior useful or harmful for optimizations? Comment Actions fastisel also uses ArgListEntry. Comment Actions remove outdatead comment Comment Actions lgtm Skimming through the test cases, they all look like human written IR where the author forgot to put the attribute on the call site. There is a risk that this could break a frontend that doesn't place ABI attributes on the call site. I think the new behavior is right, but go ahead and add something to llvm/docs/ReleaseNotes.rst to document the behavior change. Comment Actions We bisected an MSan report to this: https://bugs.chromium.org/p/chromium/issues/detail?id=1209013 The code the msan diag is for doesn't look obviously incorrect, so maybe this doesn't synergize with msan instrumentation? Comment Actions I looks like this patch, when committed as 1c7f32334d4becc725b9025fd32291a0e5729acd, caused two additional build bot failures on s390x: I see that https://reviews.llvm.org/D102667 was an attempt to fix problems with this patch for msan, but it addressed only the zeroext attribute. On s390x we also often require the signext attribute, in particular for a plain "int" function argument or return value. I have not looked into this in detail, but I see in msan_interface_internal.h quite a number of routines using "int", so it wouldn't surprise me if this could explain the regression ... Comment Actions I may be misunderstanding your comment, but that patch added zeroext to parameters of msan runtime calls, it has nothing to do with handling of zeroext/signext of existing calls in the IR. If this patch is actually causing an issue, either some instrumentation isn't properly setting ABI parameter attributes on calls that the instrumentation introduces (e.g. calls to runtime functions/intrinsics), or the frontend didn't properly handle ABI parameter attributes in the first place (which is unlikely for Clang due to indirect calls working for many years). To debug the msan issue I saw, I diffed object files, then looked at the calls in the LLVM IR for differing functions to see if any of their parameter ABI attributes were mismatched with the callee's. Since I don't have a s390x machine, I'm not sure how to debug the failures. Comment Actions if the msan runtime functions already required signext to work, then this change wouldn't have done anything since the msan runtime functions created in MemorySanitizer.cpp were never marked with signext. See MaybeWarningFn and MaybeStoreOriginFn in MemorySanitizer.cpp Comment Actions The test failures are from the libfuzzer tests, which suggests that the coverage instrumentation pass has the same problem that msan had. I see an instance of the ZExt attribute there, that doesn't match the call site: The call site doesn't appear to set the attribute appropriately: Just a wild guess. Comment Actions hmm I thought I had checked all the instrumentation passes for this issue, but I guess not Comment Actions Hmm, the https://reviews.llvm.org/D103288 commit did fix the problem on s390x again. Thanks! Comment Actions Unfortunately, I now see this patch also caused another regression, this time in the s390x multistage builder. Again, this was initially hidden by other regressions; now that these are resolved, you can see the problems here (https://lab.llvm.org/buildbot/#/builders/8/builds/1277): Comparing the disassembly of the stage2 ADTTests executable before and after this commit, I see that after this commit, some C runtime library calls are made without properly (sign-)extending their arguments. Specifically, and this explains the reported bugs, the "int" argument in a call to ldexp is no longer sign-extended as it needs to be. Also (this is probably not related to this bug, but I noticed in the assembly), the "int" argument in a call to fputc is no longer sign-extended. Both cases appear to be calls that were synthesized by the LLVM middle-end (as opposed to being emitted by the clang front-end). I notice that the emitLibCall routine in lib/Transforms/Utils/BuildLibCalls.cpp calls inferLibFuncAttributes that will set various of these attributes, including the signext attributes, but it will set them only on the function, not on the call itself. This seems to contradict the very assumption of this patch. (But note that I'm not sure if this is the only place that emits library function calls.) Maybe it would be good to revert this again until all these issues are sorted out? I would really like to get the s390x build bots all green again soon ... Comment Actions Sorry for all the issues, reverted. Comment Actions I believe I've fixed all the remaining issues. I'll keep an eye on https://lab.llvm.org/buildbot/#/builders/8 after relanding this. | ||||||||||
I think this also applies to all the attributes above. I assume that doing this to every attribute check may require updating more tests, but I think it's the right thing to do in the long run. I can live with a FIXME comment for now, but I want the code to document that it's the ABI attributes on the call site that matter, not the callee.