Page MenuHomePhabricator

[MLIR] Continue renaming of "SideEffects"
ClosedPublic

Authored by stephenneuendorffer on May 13 2020, 10:36 AM.

Details

Summary

MLIRSideEffects -> MLIRSideEffectInterfaces
SideEffects.h -> SideEffectInterfaces.h
SideEffects.cpp -> SideEffectInterface.cpp

Note that I haven't renamed TableGen/SideEffects.h or TableGen/SideEffects.cpp

find -name "*.h" -exec sed -i "s/SideEffects.h/SideEffectInterfaces.h/" "{}" \;
find -name "CMakeLists.txt" -exec sed -i "s/MLIRSideEffects/MLIRSideEffectInterfaces/" "{}" \;

Diff Detail

Unit TestsFailed

TimeTest
70 msLLVM.CodeGen/AArch64/GlobalISel::Unknown Unit Message ("")
Script: -- : 'RUN: at line 2'; c:\ws\beta\llvm-project\build\bin\llc.exe -mtriple=aarch64-darwin-ios13 -O0 -global-isel -stop-after=irtranslator -verify-machineinstrs -o - C:\ws\beta\llvm-project\llvm\test\CodeGen\AArch64\GlobalISel\irtranslator-inline-asm.ll | c:\ws\beta\llvm-project\build\bin\filecheck.exe C:\ws\beta\llvm-project\llvm\test\CodeGen\AArch64\GlobalISel\irtranslator-inline-asm.ll

Event Timeline

stephenneuendorffer edited the summary of this revision. (Show Details)
mehdi_amini added inline comments.May 14 2020, 9:53 AM
mlir/lib/Interfaces/SideEffectInterfaces.cpp
1

Please fix :)

stephenneuendorffer marked an inline comment as done.May 14 2020, 2:49 PM
mehdi_amini accepted this revision.May 14 2020, 7:18 PM
This revision was not accepted when it landed; it landed in state Needs Review.May 15 2020, 2:43 PM
This revision was automatically updated to reflect the committed changes.

I would have really preferred that we went in the opposite direction.

I would have really preferred that we went in the opposite direction.

+1, I mentioned it on a previous revision: duplicating "Interface" in the path isn't useful.
I understood it would be coming later but first this was just aligning it all. @stephenneuendorffer can you clarify?

@mehdi_amini @rriddle There were multiple commenters who thought it was better to be consistent even if there was further mass renaming involved:
see https://reviews.llvm.org/D79517

mehdi_amini added a comment.EditedMay 27 2020, 11:11 PM

@mehdi_amini @rriddle There were multiple commenters who thought it was better to be consistent even if there was further mass renaming involved:
see https://reviews.llvm.org/D79517

Sure but my assumption is that the mass renaming is coming... Otherwise I'd have push back here.
(But I actually didn't understand why "being consistent" was preventing from renaming in the other direction in the first place....)