[PATCH] D133949: Make sure the right parameter extension attributes are added in various instrumentation passes.

[Eli Friedman via Phabricator via llvm-commits]

efriedma added a comment.

It sounds like



================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:357
+          FnAS = FnAS.addAttribute(Ctx, AK);
+    }
+    else
----------------
`} else {`


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:344
+  // Add AS to FnAS while taking special care with integer extensions.
+  auto addAttrSet = [&](AttributeSet &FnAS, const AttributeSet &AS,
+                        bool Param = true) -> void {
----------------
jonpa wrote:
> efriedma wrote:
> > Can we mess with the way this works so that the "attributes" from OMPKinds.def aren't directly LLVM IR attributes?  I think the current version has the right semantics, but using zext/sext as "placeholder" attributes is really confusing.  (Maybe instead of writing AttributeSet constructors in OMPKinds.def, make the names from OMP_ATTRS_SET into an enum, and put the actual code to translate from those enums to LLVM IR attributes here.)
> It says at the top of OMPKinds.def "This file is under transition to OMP.td with TableGen code generation.", so perhaps we could follow the current use of AttributeSet constructors in this patch for now..?
> 
> Personally, I haven't thought about this as a problem, even though the whole #include scheme isn't exactly super readable to me. Would probably be easier to work with using TableGen... 
> 
> 
I don't think it's really related to whether we use TableGen; either way, it's still bits of C++ code, and the question is just whether we put those bits into the header, or in a switch statement in this function.  But I won't push too hard for this.


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

https://reviews.llvm.org/D133949