fhahn added inline comments.
================
Comment at: clang/lib/CodeGen/CGCall.cpp:2535
ArgAttrs[FirstIRArg + i] =
llvm::AttributeSet::get(getLLVMContext(), Attrs);
}
----------------
nlopes wrote:
> sammccall wrote:
> > fhahn wrote:
> > > nlopes wrote:
> > > > fhahn wrote:
> > > > > nlopes wrote:
> > > > > > ab wrote:
> > > > > > > Hmm, if I'm reading this right, this overwrites the `nonnull
> > > > > > > dereferenceable align` attributes separately computed for `this`
> > > > > > > around l2335, right? (or `inalloca` and `sret` before that)
> > > > > > >
> > > > > > > It sounds like an ancient bug, that's only exposed because
> > > > > > > `noundef` ends up triggering this logic much more often?
> > > > > > >
> > > > > > > Many of our downstream tests hit this. The hacked up patch seems
> > > > > > > to do the job; ideally we'd feed the previously-computed attrs
> > > > > > > when constructing the AttrBuilder (which would also fix the
> > > > > > > padding argument), but we'd need to match up the IR args first.
> > > > > > > Maybe that's fine as a special-case for arg 0 though
> > > > > > nice catch! It's an ancient bug where arg 0 is overwritten.
> > > > > Is anybody looking into a fix or should we revert the patch until
> > > > > this can be properly addressed?
> > > > I would recommend against reverting this patch because of all the
> > > > followup patches. There were quite a few commits afterwards plus fixes
> > > > to buildbot configurations, so it's a non-trivial overhead to revert
> > > > everything.
> > > > I was assuming @ab would fix it as he already root-caused the bug..
> > > FWIW it seems a bit unfortunate that there are some clear regressions
> > > when the tests got update, e.g.
> > > https://github.com/llvm/llvm-project/commit/1b1c8d83d3567a60280291c0adb95d1d60335509#diff-7e80d593f26f6f6fb24765c6a169884d7350685d565ee970b0a7b9abaf0fb205L355
> > >
> > > I'll work with @ab to fix this though rather than reverting, because
> > > another revert would cause even more conflicts for us downstream.
> > > FWIW it seems a bit unfortunate that there are some clear regressions
> > > when the tests got update, e.g.
> > > https://github.com/llvm/llvm-project/commit/1b1c8d83d3567a60280291c0adb95d1d60335509#diff-7e80d593f26f6f6fb24765c6a169884d7350685d565ee970b0a7b9abaf0fb205L355
> > >
> > > I'll work with @ab to fix this though rather than reverting, because
> > > another revert would cause even more conflicts for us downstream.
> >
> > Just a reminder that the 14 release is cut soon (1 feb:
> > https://llvm.discourse.group/t/llvm-14-0-0-release-schedule/5846)
> >
> > I don't know this code; if a clean fix is ready soon and unlikely to have a
> > ripple effect then great. But it does seem risky to be treating such
> > configuration changes as "too big to fail" at this point in the release
> > cycle.
> Thank you Florian!
should be fixed by 67aa314bcee7
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D105169/new/
https://reviews.llvm.org/D105169
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
