[PATCH] D34367: CodeGen: Fix address space of indirect function argument

[John McCall via Phabricator via cfe-commits]

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 be clearer.  I would say something like "The argument is semantically a load from this l-value."


================
Comment at: lib/CodeGen/CGCall.h:221
+    };
+    bool HasLV;
+
----------------
Please add an IsUsed flag so that you can assert that e.g. getRValue and/or copyInto aren't called twice.  It's okay for this to be `mutable`, since it's just a data-flow assertion rather than logically a change to the value of the argument.


================
Comment at: lib/CodeGen/CGCall.h:226
+    CallArg(RValue rv, QualType ty) : RV(rv), HasLV(false), Ty(ty) {}
+    CallArg(LValue _LV, QualType ty) : LV(_LV), HasLV(true), Ty(ty) {}
+    bool hasLValue() const { return HasLV; }
----------------
Why the two different naming conventions for these parameters?  Just call the new one `lv` for consistency, please.


================
Comment at: lib/CodeGen/CGCall.h:234
+
+    LValue getLValue() const {
+      assert(HasLV);
----------------
Please name this something like `getKnownLValue` and add a parallel `getKnownRValue`. These should assert `!IsUsed` but not set it.


================
Comment at: lib/CodeGen/CGCall.h:248
+      return HasLV ? LV.getAddress() : RV.getAggregateAddress();
+    }
+
----------------
Part of my thinking in suggesting this representation change was that the current representation was prone to a certain kind of bug where clients blindly use the RValue without realizing that it's actually something they need to copy from instead of using directly. That is generally a bug because indirect arguments are expected to be independent slots and are not permitted to alias. The new representation implicitly fixes these bugs by pushing users towards using one of these approaches.

All of this is to say that I'm not thrilled about having a getAggregateAddress here.


https://reviews.llvm.org/D34367