This is an archive of the discontinued LLVM Phabricator instance.

[RFC][GlobalISel] Remove const from most MatchTableExecutor methods
AbandonedPublic

Authored by Pierre-vh on Aug 23 2023, 5:16 AM.

Details

Summary

It's an internal API, there is no need to have const functions.
We often unintentionally work around constness anyway by using T& data members which can be modified in const functions.

Diff Detail

Unit TestsFailed

Event Timeline

Pierre-vh created this revision.Aug 23 2023, 5:16 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 23 2023, 5:16 AM
Pierre-vh requested review of this revision.Aug 23 2023, 5:17 AM
Herald added a project: Restricted Project. · View Herald Transcript

Additional context: While refactoring this area for some upcoming diffs (removing the deprecated GICombiner backend), I changed some data members from T& to T and got a lot of errors due to this.
While I usually favor const-correctness as much as possible, it's really never respected here. We always use references or pointers, which means we can always call non-const functions on those even in a const function.

Example:

struct Foo {
    int foo() {
        return 0;
    }
};

struct Bar {
    Foo &F;
    void bar() const {
        F.foo();
    }
};

So there's two solutions, the first one is for me to use mutable whenever I run into a case like this when refactoring, or we can just eliminate const where it doesn't make sense.
I'm fine with both, but I think it's cleaner to just eliminate these "fake const" that don't really guarantee anything in terms of semantics. Hence why this diff is a RFC.
e.g. we constantly mutate state - directly or indirectly - in those functions through the MIR builder, MRI, Observers, etc.

arsenm added inline comments.Aug 23 2023, 3:32 PM
llvm/lib/Target/AMDGPU/AMDGPUInstructionSelector.h
102

I'm not seeing the context how this helps. I think things should be kept const, especially since all the others here are

Additional context: While refactoring this area for some upcoming diffs (removing the deprecated GICombiner backend), I changed some data members from T& to T and got a lot of errors due to this.
While I usually favor const-correctness as much as possible, it's really never respected here. We always use references or pointers, which means we can always call non-const functions on those even in a const function.

Example:

struct Foo {
    int foo() {
        return 0;
    }
};

The foo here would just have to be const? don't see the issue

Pierre-vh planned changes to this revision.Aug 24 2023, 12:08 AM

Additional context: While refactoring this area for some upcoming diffs (removing the deprecated GICombiner backend), I changed some data members from T& to T and got a lot of errors due to this.
While I usually favor const-correctness as much as possible, it's really never respected here. We always use references or pointers, which means we can always call non-const functions on those even in a const function.

Example:

struct Foo {
    int foo() {
        return 0;
    }
};

The foo here would just have to be const? don't see the issue

It's a trivial example, in most cases we'd be calling CombinerHelper functions or GlobalISelObserver functions.
I'll double check if I can make the offending function consts, and keep mutable around for now. I'll come back to this if I still think it makes sense afterwards

Pierre-vh abandoned this revision.Aug 31 2023, 2:20 AM