[PATCH] D36800: Add rewrite by-reference parameter pass

[Siddharth via Phabricator via llvm-commits]

bollu accepted this revision.
bollu added a comment.
This revision is now accepted and ready to land.

Nit: Consider using `IRBuilder`? Not sure if this is **needed**, but maybe it avoids the extra parameter. YMMV. Other than that LGTM.



================
Comment at: lib/Transform/RewriteByReferenceParameters.cpp:42
+
+    if (Inst.getParent() == Entry)
+      return;
----------------
Could you please document why this is necessary?


================
Comment at: lib/Transform/RewriteByReferenceParameters.cpp:55
+
+    // We currently match for a very specific function. In case this proofs
+    // useful, we can make this code dependent on readonly metadata.
----------------
Nit: `this proofs useful` -> `this proves useful`


================
Comment at: lib/Transform/RewriteByReferenceParameters.cpp:65
+
+    auto *Alloca = dyn_cast<AllocaInst>(BitCast->getOperand(0));
+
----------------
Perhaps we can make this slightly nicer with the pattern matching utilities. If not, I believe we should at least use the `Operator` versions so this can work on constant versions of the operands as well.


================
Comment at: lib/Transform/RewriteByReferenceParameters.cpp:72
+        new AllocaInst(Alloca->getType()->getElementType(), 0,
+                       "polly_byref_alloca", Entry->getTerminator());
+
----------------
Consider appending the original alloca name?


================
Comment at: lib/Transform/RewriteByReferenceParameters.cpp:74
+
+    auto *LoadedVal = new LoadInst(Alloca, "polly_byref_load", &Inst);
+    new StoreInst(LoadedVal, NewAlloca, &Inst);
----------------
Consider appending the original alloca name?


================
Comment at: lib/Transform/RewriteByReferenceParameters.cpp:76
+    new StoreInst(LoadedVal, NewAlloca, &Inst);
+    auto *NewBitCast = new BitCastInst(NewAlloca, BitCast->getType(),
+                                       "polly_byref_cast", &Inst);
----------------
Consider appending the original Alloca name?


================
Comment at: test/RewriteByReferenceParameters/fortran_io.ll:27
+
+%struct.__st_parameter_dt = type { %struct.__st_parameter_common, i64, i64*, i64*, i8*, i8*, i32, i32, i8*, i8*, i32, i32, i8*, [256 x i8], i32*, i64, i8*, i32, i32, i8*, i8*, i32, i32, i8*, i8*, i32, i32, i8*, i8*, i32, [4 x i8] }
+%struct.__st_parameter_common = type { i32, i32, i8*, i32, i32, i8*, i32* }
----------------
I believe we can reduce this type to whatever we want to. Consider making this smaller?


https://reviews.llvm.org/D36800