LLVM Bugzilla is read-only and represents the historical archive of all LLVM issues filled before November 26, 2021. Use github to submit LLVM bugs

Bug 48810 - InstCombine adds incompatible attributes to call site
Summary: InstCombine adds incompatible attributes to call site
Status: RESOLVED FIXED
Alias: None
Product: libraries
Classification: Unclassified
Component: Scalar Optimizations (show other bugs)
Version: trunk
Hardware: PC All
: P enhancement
Assignee: Unassigned LLVM Bugs
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-01-19 18:26 PST by Zequan Wu
Modified: 2021-01-21 09:44 PST (History)
6 users (show)

See Also:
Fixed By Commit(s): 070af1b7887f


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Zequan Wu 2021-01-19 18:26:48 PST
Here is the minimum repro.
repro.c:
a, b;
void *mempcpy(void *, *, long);
c(*) {
  char *d = mempcpy(d, a, b);
  *d = '\0';
  c(d);
}

repro.sh:
clang -mllvm -attributor-enable=module -O2 repro.c

error:
Wrong types for attribute: inalloca nest noalias nocapture noundef nonnull readnone readonly signext zeroext byref(void) byval(void) preallocated(void) sret(void) align 1 dereferenceable(1) dereferenceable_or_null(1)
  tail call nonnull dereferenceable(1) void @llvm.memcpy.p0i8.p0i8.i64(i8* align 1 undef, i8* readonly align 1 %3, i64 %conv1, i1 false) #3
in function c
fatal error: error in backend: Broken function found, compilation aborted!
clang-12: error: clang frontend command failed with exit code 70 (use -v to see invocation)
Comment 1 Johannes Doerfert 2021-01-19 20:53:07 PST
This is a bug, but not an Attributor one ;)

https://godbolt.org/z/vhsKPf

Looking where the attribute needs to be dropped now.
Comment 2 Johannes Doerfert 2021-01-19 21:05:33 PST
Somewhere in instcombine but I didn't find it immediately, I hope someone knows and gives an update tomorrow.
Comment 3 Zequan Wu 2021-01-20 08:32:19 PST
Thanks for pointing it out. I'll give it a try.
Comment 4 Johannes Doerfert 2021-01-20 09:02:35 PST
(In reply to Zequan Wu from comment #3)
> Thanks for pointing it out. I'll give it a try.

It might be as simple as this https://reviews.llvm.org/D87306

I haven't tried it though. We might not need to know where it comes from ;)
Comment 5 Sanjay Patel 2021-01-20 11:44:34 PST
Minimal test:

define i32 @PR48810(i32* %x) {
  %r = call dereferenceable(1) i8* @mempcpy(i8* undef, i32* null, i64 undef)
  ret i32 undef
}

declare i8* @mempcpy(i8*, i32*, i64)

-------------------------------------------------------------------------------

Instcombine tries to convert that to a memcpy intrinsic:

Value *LibCallSimplifier::optimizeMemPCpy(CallInst *CI, IRBuilderBase &B) {
  Value *Dst = CI->getArgOperand(0);
  Value *N = CI->getArgOperand(2);
  // mempcpy(x, y, n) -> llvm.memcpy(align 1 x, align 1 y, n), x + n
  CallInst *NewCI =
      B.CreateMemCpy(Dst, Align(1), CI->getArgOperand(1), Align(1), N);
  NewCI->setAttributes(CI->getAttributes());

-------------------------------------------------------------------------------

But IIUC, we can't apply "dereferenceable" to a void return.
Comment 6 Johannes Doerfert 2021-01-20 12:05:47 PST
(In reply to Sanjay Patel from comment #5)
> Minimal test:
> 
> define i32 @PR48810(i32* %x) {
>   %r = call dereferenceable(1) i8* @mempcpy(i8* undef, i32* null, i64 undef)
>   ret i32 undef
> }
> 
> declare i8* @mempcpy(i8*, i32*, i64)
> 
> -----------------------------------------------------------------------------
> --
> 
> Instcombine tries to convert that to a memcpy intrinsic:
> 
> Value *LibCallSimplifier::optimizeMemPCpy(CallInst *CI, IRBuilderBase &B) {
>   Value *Dst = CI->getArgOperand(0);
>   Value *N = CI->getArgOperand(2);
>   // mempcpy(x, y, n) -> llvm.memcpy(align 1 x, align 1 y, n), x + n
>   CallInst *NewCI =
>       B.CreateMemCpy(Dst, Align(1), CI->getArgOperand(1), Align(1), N);
>   NewCI->setAttributes(CI->getAttributes());
> 
> -----------------------------------------------------------------------------
> --
> 
> But IIUC, we can't apply "dereferenceable" to a void return.

We need something like this (taken from InstCombineCalls.cpp:2281):

``` 
    // Get any return attributes.
    AttrBuilder RAttrs(CallerPAL, AttributeList::ReturnIndex);
    
    // If the return value is not being used, the type may not be compatible
    // with the existing attributes.  Wipe out any problematic attributes.
    RAttrs.remove(AttributeFuncs::typeIncompatible(NewRetTy));  
```
Comment 7 Sanjay Patel 2021-01-20 12:09:59 PST
Thanks for the pointer!
Zequan, are you working on this? If not, I can post a patch for review.
Comment 8 Zequan Wu 2021-01-20 12:11:42 PST
No, I am not. 
Feel free to post a patch. Thanks
Comment 9 Sanjay Patel 2021-01-20 13:50:52 PST
https://reviews.llvm.org/D95088
Comment 10 Sanjay Patel 2021-01-21 06:56:22 PST
Minimally fixed the crash:
https://reviews.llvm.org/rG070af1b7887f
Comment 11 Zequan Wu 2021-01-21 09:44:38 PST
Thanks for the quick fix.