[PATCH] D48106: implemented proto to llvm

Matt Morehouse via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Jun 12 18:29:58 PDT 2018


morehouse added a comment.

Where is the fuzz target?



================
Comment at: tools/clang-fuzzer/proto-to-llvm/loop_proto_to_llvm.cpp:33
+int ptr_ctr = 0;
+int val_ctr = 0;
+
----------------
I'd suggest wrapper functions that return unused variable names, so your code below won't need to juggle these globals.


================
Comment at: tools/clang-fuzzer/proto-to-llvm/loop_proto_to_llvm.cpp:43
+std::ostream &operator<<(std::ostream &os, const VarRef &x) {
+  ptr_ctr++;
+  switch (x.arr()) {
----------------
The way these globals are being used seems confusing and error-prone.

Maybe what you want is to replace the operator<< overloads with functions that emit the LLVM and then return the variable name that the result is stored in.  Then the parent node can use the returned variable name without juggling globals.


================
Comment at: tools/clang-fuzzer/proto-to-llvm/loop_proto_to_llvm.cpp:48
+              << "%m_" << ptr_ctr << " = mul i64 8, %ct\n"
+              << "%i_" << ptr_ctr << " = add i64 %p2i_" << ptr_ctr << ", %m_"
+              << ptr_ctr << "\n"
----------------
`getelementptr` should make this much simpler.


================
Comment at: tools/clang-fuzzer/proto-to-llvm/loop_proto_to_llvm.cpp:80
+            << "%val_" << val_ctr << " = load i32, i32* %pval_" << val_ctr
+            << "\n";
+}
----------------
When will this last return be executed?


================
Comment at: tools/clang-fuzzer/proto-to-llvm/loop_proto_to_llvm.cpp:132
+  int p = ptr_ctr + 1;
+  return os << x.varref() << x.rvalue() << "store i32 %val_" << val_ctr
+            << ", i32* %i2p_" << p << "\n";
----------------
Probably not a big deal, but maybe load the rvalue first?  I'd expect the frontend to do that normally to simplify register allocation.


================
Comment at: tools/clang-fuzzer/proto-to-llvm/loop_proto_to_llvm.cpp:132
+  int p = ptr_ctr + 1;
+  return os << x.varref() << x.rvalue() << "store i32 %val_" << val_ctr
+            << ", i32* %i2p_" << p << "\n";
----------------
morehouse wrote:
> Probably not a big deal, but maybe load the rvalue first?  I'd expect the frontend to do that normally to simplify register allocation.
Also, I think it is easier to read if you put each statement on its own line.


================
Comment at: tools/clang-fuzzer/proto-to-llvm/loop_proto_to_llvm.cpp:144
+std::ostream &operator<<(std::ostream &os, const LoopFunction &x) {
+  return os << "define void @foo(i32* %a, i32*%b, i32* noalias %c, i64 %s) {\n"
+            << "%i = alloca i64\n"
----------------
Space before `%b`.


================
Comment at: tools/clang-fuzzer/proto-to-llvm/loop_proto_to_llvm.cpp:153
+            << "body:\n"
+            << x.statements() << "%z = add i64 1, %ct\n"
+            << "store i64 %z, i64* %i\n"
----------------
Can we put `%z = ...` on the next line for consistency?


================
Comment at: tools/clang-fuzzer/proto-to-llvm/loop_proto_to_llvm.cpp:156
+            << "br label %loop\n"
+            << "\nendloop:\n"
+            << "ret void\n}\n";
----------------
Can we put the first newline on previous statement for consistency?


Repository:
  rC Clang

https://reviews.llvm.org/D48106





More information about the llvm-commits mailing list