jdoerfert added inline comments.
================ Comment at: clang/lib/Sema/SemaChecking.cpp:4489 + // Alignment calculations can wrap around if it's greater than 2**29. + unsigned MaximumAlignment = 536870912; + if (I > MaximumAlignment) ---------------- hfinkel wrote: > jdoerfert wrote: > > erichkeane wrote: > > > jdoerfert wrote: > > > > erichkeane wrote: > > > > > I thought we had this stored somewhere else? We probably should have > > > > > this be a constant somewhere in the frontend. I THINK I remember > > > > > doing a review where I pulled this value into clang somewhere... > > > > That was D72998, and I don't think Clang is the right place for this > > > > constant. It is a property of the llvm alignment attribute and it > > > > should live there. Thus, llvm/include/Attributes.h or some similar > > > > place. Can't we "fix" the linker error by making it a constexpr global > > > > or are the errors because of other file content? If the latter, we > > > > could go with a llvm/include/magic_constants.h ;) > > > The one I was thinking of was this one: https://reviews.llvm.org/D68824 > > > > > > I don't remember what we came up with on the linking issue. It would be > > > really nice if it was just something included from LLVM, but I think SEMA > > > typically doesn't include stuff from LLVM either. > > I'm not too happy with the duplication of the constant but defining it once > > in clang is certainly better than having it in N places. For OpenMP we look > > into LLVM during SEMA and here there is an argument to be made that we > > should as well. I imagine more cases would pop up over time. > > > > FWIW, if we allow to include LLVM headers, e.g., from IR or Frontend, we > > could still have a wrapper in SEMA to get the information so it doesn't > > expose the llvm:: namespace at the use sides (if that helps). > > For OpenMP we look into LLVM during SEMA > > How do we do that? > > There's certainly an interesting philosophical issue around whether changes > in LLVM should directly manifest as Clang behavioral changes, especially in > -fsyntax-only. The answer to this question might be different for extensions > vs. core language features (although alignment restrictions might implicate > both). AFAIKT, historically , our answer has been to insist on separation. > > For OpenMP we look into LLVM during SEMA > How do we do that? I was referring to code like this https://reviews.llvm.org/D71830#C1739755NL11085 which is in CodeGen right now but has to move to SemaOverload. The code is completely reusable between Clang and Flang so I put it in lib/Frontend/OpenMP and I think that is the right place for it. > There's certainly an interesting philosophical issue around whether changes > in LLVM should directly manifest as Clang behavioral changes, especially in > -fsyntax-only. The answer to this question might be different for extensions > vs. core language features (although alignment restrictions might implicate > both). AFAIKT, historically , our answer has been to insist on separation. I get that in a general sense. For the problem at hand, and as far as I known, the restriction stems only from the LLVM-IR restriction, correct? If so, what is the argument for separation? I mean, a change of the value in LLVM might directly impact Clang behavior. I could also see us clamping the alignment during codegen. While that might have other problems they seem less practical to me. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72996/new/ https://reviews.llvm.org/D72996 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
