[PATCH] D36556: [IR] AutoUpgrade ModuleFlagBehavior for PIC and PIE level

[Duncan P. N. Exon Smith via Phabricator via llvm-commits]

dexonsmith added a comment.

Who changed the behaviour?  Can we get them to review the behaviour of the upgrade?

I don't see a general test for PIE level.  Can we add one of those as well?



================
Comment at: lib/IR/AutoUpgrade.cpp:2269-2270
+  assert(MD->getNumOperands() > 2 && "Module Flag need to have at least 3 Ops");
+  if (ConstantInt *Behavior =
+          mdconst::dyn_extract_or_null<ConstantInt>(MD->getOperand(0))) {
+    if (Behavior->getLimitedValue() == Before) {
----------------
This would read more nicely as `auto *Behavior`, since you've already been explicit about the `ConstantInt`.


================
Comment at: lib/IR/AutoUpgrade.cpp:2273-2274
+      Type *Int32Ty = Type::getInt32Ty(Context);
+      MD->replaceOperandWith(
+          0, ConstantAsMetadata::get(ConstantInt::get(Int32Ty, After)));
+      return true;
----------------
It's not generally sound to call `MDNode::replaceOperandWith`, unless the nodes are "distinct".  I strongly suspect these nodes are "uniqued", which means that they are semantically constants (with far-too-lenient an API).

Instead, you should construct a new node to attach to the `NamedMDNode`.


https://reviews.llvm.org/D36556