fpetrogalli added a comment. In D137517#4045337 <https://reviews.llvm.org/D137517#4045337>, @jrtc27 wrote:
> In D137517#4045315 <https://reviews.llvm.org/D137517#4045315>, @fpetrogalli > wrote: > >> In D137517#4045299 <https://reviews.llvm.org/D137517#4045299>, @jrtc27 wrote: >> >>> In D137517#4042875 <https://reviews.llvm.org/D137517#4042875>, @fpetrogalli >>> wrote: >>> >>>> In D137517#4042758 <https://reviews.llvm.org/D137517#4042758>, >>>> @fpetrogalli wrote: >>>> >>>>> After submitting this, I had to revert it >>>>> <https://github.com/llvm/llvm-project/commit/8bd65e535fb33bc48805bafed8217b16a853e158> >>>>> because of failures like >>>>> https://lab.llvm.org/buildbot/#/builders/225/builds/12367/steps/5/logs/stdio >>>> >>>> I have resubmitted with what I hope is the right fix (I could not >>>> reproduce any of the failures I was seeing in buildbot, on my machine the >>>> build is fine). >>>> >>>> The new commit is at >>>> https://github.com/llvm/llvm-project/commit/ac1ffd3caca12c254e0b8c847aa8ce8e51b6cfbf >>>> - in the commit message I have explained what I have changed WRT this >>>> original patch. I have added the >>>> tablegen target `RISCVTargetParserTableGen` in the `DEPENDS` list of >>>> `clangDriver` and `clangBasic`. This makes sure that the `.*inc` file with >>>> theist o the CPU is available even if `LLVMTargetParser` has not been >>>> built yet. >>> >>> But you didn't use the proper Differential Revision tag, so the diff here >>> didn't get updated to reflect the amended version committed :( >> >> What should I have done? Add the `Differential Revision: >> https://reviews.llvm.org/D137517` as the last line of the commit with the >> rework? I wasn't aware of this for reworks. > > Yes, it should have the same trailer as the original commit, otherwise it > won't be correctly tracked by Phabricator. A "rework" isn't special, it's > revert, reopen the revision, update the revision and land the revision again. > If re-review isn't needed then you can skip some of the middle, but that's > it. Though in this case I do think re-review was warranted, the new clang > dependency seems a bit dubious and hints at the design not being quite right. My bad - I assumed that adding such tablegenning dependency in clang was a minor thing. I'll see if I can remove it. Any suggestions on how this could be done without duplicating the information in RISCV.td? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137517/new/ https://reviews.llvm.org/D137517 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
