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: > > 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. > > > > > > > > > > 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. > > Fair, but that's a library designed to be a home for cross-language frontend > components. The variant-selection logic to which you're referring, itself, > does not actually need to link to LLVM's IR library, correct? > > > 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. > > Yes, I believe that the restriction is necessary because of an underlying > LLVM IR restriction. From my perspective, your argument is perfectly > rational. Clang only supports code generation using LLVM IR, and a > restriction that comes from LLVM should be directly tied to the underlying > LLVM threshold regardless of where it is surfaced. We have, however, avoided > a linking dependence (I believe, primarily, to help the load times and file > sizes of tools based on Clang which don't otherwise need to link to the LLVM > IR libraries). > The variant-selection logic to which you're referring, itself, does not > actually need to link to LLVM's IR library, correct? Correct. > We have, however, avoided a linking dependence (I believe, primarily, to help > the load times and file sizes of tools based on Clang which don't otherwise > need to link to the LLVM IR libraries). That makes total sense to me. The idea of a separate header for magic constants, e.g., `llvm/include/llvm/IR/IRConstants.h` or `llvm/include/llvm/Frontend/Constants.h`, was to prevent any linking issues. Anyway, if people feel having the constant defined twice is better for now I won't object. 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
