[PATCH] D102726: [IR][AutoUpgrade] Drop alignment from non-pointer parameters and returns

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

First, thanks a lot for looking into this again. I appreciate it a lot.

I left a bunch of minor comments and suggestions. Now that I thought about it for a second, I feel we should not look for alignment at all but instead call `typeIncompatible` here. For one, that will make future updates obsolete. It also will directly handle other attributes we might have missed here. Finally, it will handle pointer vectors and such properly.

WDYT?



================
Comment at: llvm/lib/Bitcode/Reader/BitcodeReader.cpp:5643
+          CI->removeParamAttr(ArgNo, Attribute::Alignment);
+      }
     }
----------------
I didn't see this before but CallInst is not enough, you want CallBase.
I'm not sure why you go through the functiontype but it seems odd.
If the call base type is not a pointer, just remove alignment attribute (no need to check I think).
You want the call site arguments not the function type parameters, the former are potentially more for varargs functions and then we also don't want to have aligned non-pointer arguments.
And again, no need to check if the argument is present, removing it eagerly should be as expensive.


================
Comment at: llvm/lib/IR/AutoUpgrade.cpp:4387-4392
+  for (unsigned ArgNo = 0; ArgNo < F.getFunctionType()->getNumParams();
+       ++ArgNo) {
+    if (!F.getFunctionType()->getFunctionParamType(ArgNo)->isPointerTy() &&
+        F.hasParamAttribute(ArgNo, Attribute::Alignment))
+      F.removeParamAttr(ArgNo, Attribute::Alignment);
+  }
----------------
I haven't run this but should be easier to read.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102726/new/

https://reviews.llvm.org/D102726