[PATCH] D68862: [ARM] Allocatable Global Register Variables for ARM

carwil added a comment. Reverted. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D68862/new/ https://reviews.llvm.org/D68862 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68862: [ARM] Allocatable Global Register Variables for ARM

carwil added a comment. Hi @efriedma, thanks your comments. You're right, that was hasty of me. Apologies for that, it won't happen again. RE: R6 Case in point: you're right. We're definitely not handling this correctly. I'll revert the patch, or a

[PATCH] D68862: [ARM] Allocatable Global Register Variables for ARM

efriedma added a comment. Approval is not ever a time-based process; someone appropriate actually has to do the work of reviewing the patch. If a patch doesn't get reviewed, you "ping" it a couple times, to note that you're waiting for a review. If it still isn't reviewed at that point, and y

[PATCH] D68862: [ARM] Allocatable Global Register Variables for ARM

carwil accepted this revision. carwil added a comment. This revision is now accepted and ready to land. I think there's been plenty of time for comments here. LGTM. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D68862/new/ https://reviews.llvm.org/D68862 __

[PATCH] D68862: [ARM] Allocatable Global Register Variables for ARM

anwel updated this revision to Diff 229531. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D68862/new/ https://reviews.llvm.org/D68862 Files: clang/docs/ClangCommandLineReference.rst clang/include/clang/Basic/DiagnosticDriverKinds.td clang/include/clang/Basic/DiagnosticGroups.td cl

[PATCH] D68862: [ARM] Allocatable Global Register Variables for ARM

anwel updated this revision to Diff 229296. anwel added a comment. Change clang's error message when trying to use the target's frame pointer as GRV to sound more like an error then a warning. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D68862/new/ https://reviews.llvm.org/D68862 Fil

[PATCH] D68862: [ARM] Allocatable Global Register Variables for ARM

anwel updated this revision to Diff 229269. anwel added a comment. Rebase on current llvm-project master CHANGES SINCE LAST ACTION https://reviews.llvm.org/D68862/new/ https://reviews.llvm.org/D68862 Files: clang/docs/ClangCommandLineReference.rst clang/include/clang/Basic/DiagnosticDriv

[PATCH] D68862: [ARM] Allocatable Global Register Variables for ARM

carwil added a comment. Just want to double check with @efriedma, before we except this. I believe this patch now catches all the points you made on https://reviews.llvm.org/D56005. Anything we've missed? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D68862/new/ https://reviews.llvm.or

[PATCH] D68862: [ARM] Allocatable Global Register Variables for ARM

anwel added inline comments. Comment at: clang/lib/Basic/Targets/ARM.cpp:903-907 + for (std::string &Feature : Features) { +if (Feature.compare(SearchFeature) == 0) + return true; + } + return false; chill wrote: > This explicit loop can be written li

[PATCH] D68862: [ARM] Allocatable Global Register Variables for ARM

anwel updated this revision to Diff 227094. anwel marked 9 inline comments as done. anwel added a comment. Rebase and make some variables const CHANGES SINCE LAST ACTION https://reviews.llvm.org/D68862/new/ https://reviews.llvm.org/D68862 Files: clang/docs/ClangCommandLineReference.rst c

[PATCH] D68862: [ARM] Allocatable Global Register Variables for ARM

carwil added a comment. In D68862#1708132 , @chill wrote: > In D68862#1708079 , @carwil wrote: > > > > IMHO, since reserved registes are per-function, this strongly suggests > > > implementation as function attribu

[PATCH] D68862: [ARM] Allocatable Global Register Variables for ARM

chill added a comment. In D68862#1708079 , @carwil wrote: > > IMHO, since reserved registes are per-function, this strongly suggests > > implementation as function attribute(s), rather than subtarget features > > (also for the pre-existing r9). > > What

[PATCH] D68862: [ARM] Allocatable Global Register Variables for ARM

anwel updated this revision to Diff 224862. anwel added a comment. Applied some minor changes suggested in the comments, including renaming the array of reserved registers. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D68862/new/ https://reviews.llvm.org/D68862 Files: clang/docs/Cla

[PATCH] D68862: [ARM] Allocatable Global Register Variables for ARM

carwil added a comment. > IMHO, since reserved registes are per-function, this strongly suggests > implementation as function attribute(s), rather than subtarget features (also > for the pre-existing r9). What do you mean reserved registers are per-function? That sounds like you're describing

[PATCH] D68862: [ARM] Allocatable Global Register Variables for ARM

SjoerdMeijer added inline comments. Comment at: clang/lib/Basic/Targets/ARM.cpp:902 + std::vector &Features = getTargetOpts().Features; + std::string SearchFeature = "+reserve-" + RegName.str(); + for (std::string &Feature : Features) { chill wrote: > SjoerdMe

[PATCH] D68862: [ARM] Allocatable Global Register Variables for ARM

chill added inline comments. Comment at: clang/lib/Basic/Targets/ARM.cpp:902 + std::vector &Features = getTargetOpts().Features; + std::string SearchFeature = "+reserve-" + RegName.str(); + for (std::string &Feature : Features) { SjoerdMeijer wrote: > I was po

[PATCH] D68862: [ARM] Allocatable Global Register Variables for ARM

chill added a comment. TBH, I quite dislike the creeping abuse of `SubtargetFeature`s as code generation options. cf. Target.td:1477 //===--===// // SubtargetFeature - A characteristic of the chip set. // IMHO, since r

[PATCH] D68862: [ARM] Allocatable Global Register Variables for ARM

SjoerdMeijer added a comment. Bit of a drive-by comment, but I can't say I am big fan of all the string matching on the register names. Not sure if this is a fair comment, because I haven't looked closely at it yet, but could we use more the `ARM::R[0-9]` values more? Perhaps that's difficult f

[PATCH] D68862: [ARM] Allocatable Global Register Variables for ARM

anwel created this revision. anwel added reviewers: carwil, amilendra_arm, phosek, michaelplatings, efriedma. anwel added projects: LLVM, clang. Herald added subscribers: llvm-commits, cfe-commits, hiraditya, kristof.beyls. This patch combines two earlier patches aiming at providing the same suppo