This revision was automatically updated to reflect the committed changes.
Closed by commit rL326946: CodeGen: Fix address space of indirect function
argument (authored by yaxunl, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D34367?vs
rjmccall accepted this revision.
rjmccall added a comment.
This revision is now accepted and ready to land.
Thanks. LGTM!
https://reviews.llvm.org/D34367
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/
yaxunl updated this revision to Diff 137460.
yaxunl marked an inline comment as done.
yaxunl edited the summary of this revision.
yaxunl added a comment.
Added comment about emit non-null argument check.
https://reviews.llvm.org/D34367
Files:
lib/CodeGen/CGAtomic.cpp
lib/CodeGen/CGCall.cpp
rjmccall added inline comments.
Comment at: lib/CodeGen/CGCall.cpp:3427
(void)InitialArgSize;
-RValue RVArg = Args.back().RV;
-EmitNonNullArgCheck(RVArg, ArgTypes[Idx], (*Arg)->getExprLoc(), AC,
-ParamsToSkip + Idx);
-// @llvm.objectsize s
yaxunl updated this revision to Diff 137421.
yaxunl marked an inline comment as done.
yaxunl added a comment.
Revised by John's comments. Removed CallArg::getAggregateAddress().
https://reviews.llvm.org/D34367
Files:
lib/CodeGen/CGAtomic.cpp
lib/CodeGen/CGCall.cpp
lib/CodeGen/CGCall.h
l
yaxunl marked 20 inline comments as done.
yaxunl added inline comments.
Comment at: lib/CodeGen/CGCall.h:248
+ return HasLV ? LV.getAddress() : RV.getAggregateAddress();
+}
+
rjmccall wrote:
> Part of my thinking in suggesting this representation change
rjmccall added a comment.
I'm sorry, I did a review of your patch on Phabricator but apparently never
submitted it.
Comment at: lib/CodeGen/CGCall.h:219
+ RValue RV;
+ LValue LV; /// The l-value from which the argument is derived.
+};
This could
yaxunl updated this revision to Diff 137279.
yaxunl added a comment.
Clean up CallArg::getRValue() so that it always return independent value.
https://reviews.llvm.org/D34367
Files:
lib/CodeGen/CGAtomic.cpp
lib/CodeGen/CGCall.cpp
lib/CodeGen/CGCall.h
lib/CodeGen/CGClass.cpp
lib/CodeGe
yaxunl updated this revision to Diff 136301.
yaxunl added a comment.
Revised by John's comments.
Added CallArg::copyInto and modified CallArg::getRValue() to return an
independent r-value by default. However some cases expecting l-value not
copied, therefore I added an optional argument to Call
yaxunl updated this revision to Diff 135499.
yaxunl added a comment.
sync to ToT.
https://reviews.llvm.org/D34367
Files:
lib/CodeGen/CGAtomic.cpp
lib/CodeGen/CGCall.cpp
lib/CodeGen/CGCall.h
lib/CodeGen/CGClass.cpp
lib/CodeGen/CGDecl.cpp
lib/CodeGen/CGExprCXX.cpp
lib/CodeGen/CGGPUB
rjmccall added inline comments.
Comment at: lib/CodeGen/CGCall.cpp:3446
+ return LV.asAggregateRValue();
+}
+
No, I don't think this is right. This method should always return an
independent RValue; if the CallArg is storing an LValue, it should copy from it
yaxunl updated this revision to Diff 135474.
yaxunl added a comment.
Revised by John's comments.
https://reviews.llvm.org/D34367
Files:
lib/CodeGen/CGAtomic.cpp
lib/CodeGen/CGCall.cpp
lib/CodeGen/CGCall.h
lib/CodeGen/CGClass.cpp
lib/CodeGen/CGDecl.cpp
lib/CodeGen/CGExprCXX.cpp
lib
yaxunl updated this revision to Diff 135301.
yaxunl added a comment.
Replace Flag with LValue in CallArg.
https://reviews.llvm.org/D34367
Files:
lib/CodeGen/CGCall.cpp
lib/CodeGen/CGCall.h
lib/CodeGen/CGClass.cpp
lib/CodeGen/CGDecl.cpp
lib/CodeGen/CGObjCGNU.cpp
lib/CodeGen/ItaniumCX
rjmccall added inline comments.
Comment at: lib/CodeGen/CGCall.cpp:3510
+ args.add(RValue::getAggregate(Dest.getAddress()), type, L);
}
return;
Please move all this conditionality to the call-emission code; just check
whether you have a load of an
yaxunl added inline comments.
Comment at: lib/CodeGen/CGCall.cpp:3505
+if (AS != LangAS::Default && AS != LangAS::opencl_private &&
+AS != CGM.getASTAllocaAddressSpace())
+ Flag = CallArg::ByValArgNeedsCopy;
rjmccall wrote:
> This is an odd condi
rjmccall added inline comments.
Comment at: lib/CodeGen/CGCall.cpp:3505
+if (AS != LangAS::Default && AS != LangAS::opencl_private &&
+AS != CGM.getASTAllocaAddressSpace())
+ Flag = CallArg::ByValArgNeedsCopy;
This is an odd condition. What are
yaxunl updated this revision to Diff 135109.
yaxunl added a comment.
Extends NeedsCopy of CallArg for indirect byval arguments.
https://reviews.llvm.org/D34367
Files:
lib/CodeGen/CGCall.cpp
lib/CodeGen/CGCall.h
lib/CodeGen/CGClass.cpp
lib/CodeGen/CGDecl.cpp
lib/CodeGen/CGObjCGNU.cpp
yaxunl added a comment.
In https://reviews.llvm.org/D34367#1009021, @rjmccall wrote:
> That's not really okay; there are some places that make guarantees about
> call-argument ordering, and in general we want that to be decided at a higher
> level, rather than having a particular order forced i
rjmccall added a comment.
That's not really okay; there are some places that make guarantees about
call-argument ordering, and in general we want that to be decided at a higher
level, rather than having a particular order forced in corner cases just to
satisfy a lower-level implementation requi
yaxunl added inline comments.
Comment at: lib/CodeGen/CGCall.h:211
+ push_back(CallArg(rvalue, type, needscopy, AS));
}
rjmccall wrote:
> Ah, I think I understand what's going on here now. "indirect byval"
> arguments include an implicit copy to the
yaxunl updated this revision to Diff 134416.
yaxunl edited the summary of this revision.
yaxunl added a comment.
Revised by John's comments. Removed address space from CallArg and added
l-value expression instead.
https://reviews.llvm.org/D34367
Files:
lib/CodeGen/CGCall.cpp
lib/CodeGen/CG
rjmccall added inline comments.
Comment at: lib/CodeGen/CGCall.h:211
+ push_back(CallArg(rvalue, type, needscopy, AS));
}
Ah, I think I understand what's going on here now. "indirect byval" arguments
include an implicit copy to the stack, and the co
yaxunl updated this revision to Diff 113443.
yaxunl added a comment.
Revised by John's comments.
https://reviews.llvm.org/D34367
Files:
lib/CodeGen/CGCall.cpp
lib/CodeGen/CGCall.h
lib/CodeGen/CGDecl.cpp
test/CodeGenCXX/amdgcn-func-arg.cpp
test/CodeGenOpenCL/addr-space-struct-arg.cl
I
yaxunl added inline comments.
Comment at: lib/CodeGen/CGCall.cpp:3861
< Align.getQuantity()) ||
(ArgInfo.getIndirectByVal() && (RVAddrSpace != ArgAddrSpace))) {
// Create an aligned temporary, and copy to it.
yaxunl wrote:
yaxunl added inline comments.
Comment at: lib/CodeGen/CGCall.cpp:3861
< Align.getQuantity()) ||
(ArgInfo.getIndirectByVal() && (RVAddrSpace != ArgAddrSpace))) {
// Create an aligned temporary, and copy to it.
rjmccall wrote
rjmccall added inline comments.
Comment at: lib/CodeGen/CGCall.cpp:3861
< Align.getQuantity()) ||
(ArgInfo.getIndirectByVal() && (RVAddrSpace != ArgAddrSpace))) {
// Create an aligned temporary, and copy to it.
yaxunl wrote
yaxunl marked an inline comment as done.
yaxunl added inline comments.
Comment at: lib/CodeGen/CGCall.cpp:3861
< Align.getQuantity()) ||
(ArgInfo.getIndirectByVal() && (RVAddrSpace != ArgAddrSpace))) {
// Create an aligned temporary, and co
yaxunl marked 6 inline comments as done.
yaxunl added inline comments.
Comment at: lib/CodeGen/CGCall.cpp:3851
+ ->getType()
+ ->getPointerAddressSpace();
const unsigned ArgAddrSpace =
--
rjmccall added inline comments.
Comment at: lib/CodeGen/CGCall.cpp:3851
+ ->getType()
+ ->getPointerAddressSpace();
const unsigned ArgAddrSpace =
yaxunl wrote:
> rjmccall wro
yaxunl marked 5 inline comments as done.
yaxunl added inline comments.
Comment at: lib/CodeGen/CGCall.cpp:3832
+ "indirect-arg-temp");
+IRCallArgs[FirstIRArg] = CastToAllocaAddrSpace(Addr.getPointer());
rjmccall wrote
rjmccall added inline comments.
Comment at: lib/CodeGen/CGCall.cpp:3832
+ "indirect-arg-temp");
+IRCallArgs[FirstIRArg] = CastToAllocaAddrSpace(Addr.getPointer());
Isn't the original code here correct? You're basical
yaxunl updated this revision to Diff 106030.
yaxunl marked an inline comment as done.
yaxunl edited the summary of this revision.
yaxunl added a comment.
Always put indirect arg in alloca addr space.
https://reviews.llvm.org/D34367
Files:
lib/CodeGen/CGCall.cpp
lib/CodeGen/CGDecl.cpp
test
yaxunl marked 3 inline comments as done.
yaxunl added inline comments.
Comment at: lib/CodeGen/CGCall.cpp:1605
+ ? CGM.getDataLayout().getAllocaAddrSpace()
+ : getContext().getTargetAddressSpace(LangAS::Default));
break;
rjmccall
rjmccall added inline comments.
Comment at: lib/CodeGen/CGCall.cpp:1605
+ ? CGM.getDataLayout().getAllocaAddrSpace()
+ : getContext().getTargetAddressSpace(LangAS::Default));
break;
yaxunl wrote:
> rjmccall wrote:
> > Everything a
yaxunl added inline comments.
Comment at: lib/CodeGen/CGCall.cpp:1605
+ ? CGM.getDataLayout().getAllocaAddrSpace()
+ : getContext().getTargetAddressSpace(LangAS::Default));
break;
rjmccall wrote:
> Everything about your reasoning
rjmccall added inline comments.
Comment at: lib/CodeGen/CGCall.cpp:1605
+ ? CGM.getDataLayout().getAllocaAddrSpace()
+ : getContext().getTargetAddressSpace(LangAS::Default));
break;
Everything about your reasoning seems to apply t
yaxunl created this revision.
The byval function argument is in alloca address space in LLVM IR. However,
during Clang codegen for C++, the address space of indirect function argument
should match its address space in the source code, even for byval argument.
This is because destructor of the byva
37 matches
Mail list logo
