DavidEGrayson added a comment.

Thanks for looking at this patch, John!  I disagreed with your comment about 
calculating the encompassing type, but I will incorporate everything else and 
make a new patch tonight or within a few days.


================
Comment at: docs/LanguageExtensions.rst:1720
@@ -1712,1 +1719,3 @@
+being stored there, and the function returns 1.  The behavior of these builtins
+is well-defined for all argument values.
 
----------------
rjmccall wrote:
> Hmm.  It's not necessarily truncation; you could meaningfully use these 
> intrinsics to add two small signed numbers and store the result in an 
> unsigned number.  It's the unique representative value modulo 2^n, where n is 
> the bit-width of the result.
OK, I'll change the documentation because I like your way of phrasing it, but I 
think truncation always gives you the unique representative value modulo 2^n.  
If I'm adding ((int8_t)-1) to ((int8_t)-1), the infinite-width result would be 
-2, which in infinite-width binary is 11...1111111111110.  Then we truncate it 
to 8 bits to get 11111110, which is 254.  And 254 is also the unique 
representative value of -2 modulo 256.


================
Comment at: lib/CodeGen/CGBuiltin.cpp:289
@@ +288,3 @@
+static std::pair<unsigned, bool>
+EncompassingIntegerType(std::vector<std::pair<unsigned, bool>> Types) {
+  assert(Types.size() > 0 && "Empty list of types.");
----------------
rjmccall wrote:
> This should just take an ArrayRef.  Also, please at least introduce a typedef 
> for your pair type, and consider just making a little struct for it.
A struct called IntegerWidthAndSignedness sounds good to me, thanks.  And I 
will learn how to use ArrayRef and use it here.

================
Comment at: lib/CodeGen/CGBuiltin.cpp:314
@@ +313,3 @@
+static std::pair<unsigned, bool>
+IntegerWidthAndSignedness(const clang::ASTContext &context,
+                          const clang::QualType Type) {
----------------
rjmccall wrote:
> getIntegerWidthAndSignedness, please.
Sounds good.  That frees up the name IntegerWidthAndSignedness to be used for 
my new struct.

================
Comment at: lib/CodeGen/CGBuiltin.cpp:1601
@@ +1600,3 @@
+    auto RITy = IntegerWidthAndSignedness(CGM.getContext(), RQTy);
+    auto EITy = EncompassingIntegerType({XITy, YITy, RITy});
+
----------------
rjmccall wrote:
> These are not the most evocative names you could have chosen. :)
> 
> Also, this strategy is correct, but it produces horrible code when the 
> operands have the same signedness and the result is different, where the 
> result would otherwise be an ordinary operation and a compare.  You really 
> want to just merge the two operands and then max with the width of the result 
> type.
I don't think that would work.  It sounds like you want me to remove RITy from 
the call to EncompassingIntegerType.  That means the ecompassing type could be 
smaller than the result type, and the arithmetic intrinsic would report an 
overflow even though the mathematical result is representable in the result 
type.

For example, if I am multiplying ((uint8_t)100) by ((uint8_t)100) and storing 
the result in a (uint16_t), the result needs to be 10000 and there is no 
overflow.  To arrive at that result, we need to convert the operands to 16-bit 
unsigned integers before multiplying them.  If we multiply them as 8-bit 
unsigned integers, the multiplication intrinsic will report an overflow and 
give a result of 16.  That seems like a messy situation and I don't see how a 
max operation would fix it.

================
Comment at: lib/CodeGen/CGBuiltin.cpp:1610
@@ +1609,3 @@
+    default:
+      llvm_unreachable("Unknown security overflow builtin id.");
+    case Builtin::BI__builtin_add_overflow:
----------------
rjmccall wrote:
> These aren't really security-specific; they're just arbitrary-precision.
I agree that these functions are not security-specific.  I just copied that 
message from a place a little bit lower in CGBuiltin.cpp where it was used to 
describe some other overflow builtins.  I am happy to remove the word 
"security" from both places.

================
Comment at: lib/CodeGen/CGBuiltin.cpp:1630
@@ +1629,3 @@
+    // Extend each operand to the encompassing type.
+    if (XQTy->isSignedIntegerType()) {
+      X = Builder.CreateSExt(X, ELTy);
----------------
rjmccall wrote:
> This is Builder.CreateIntegerCast(X, ELTy, XITy.second).
Thanks, I thought there had to be a function for that already but just could 
not find it.

================
Comment at: lib/CodeGen/CGBuiltin.cpp:1667
@@ +1666,3 @@
+    // we have to extend them before storing.
+    Q = Builder.CreateZExt(Q, OutPtr.getElementType());
+
----------------
rjmccall wrote:
> EmitToMemory.
Thanks again, will do.


Repository:
  rL LLVM

http://reviews.llvm.org/D12793



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to