This is an archive of the discontinued LLVM Phabricator instance.

[NFC][AA] Prepare to convert AliasResult to class with PartialAlias offset.
ClosedPublic

Authored by dfukalov on Mar 5 2021, 3:05 AM.

Details

Summary

Main reason is preparation to transform AliasResult to class that contains
offset for PartialAlias case.

Diff Detail

Event Timeline

dfukalov created this revision.Mar 5 2021, 3:05 AM
dfukalov requested review of this revision.Mar 5 2021, 3:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 5 2021, 3:05 AM
dfukalov added inline comments.
llvm/lib/Target/AMDGPU/AMDGPUAliasAnalysis.cpp
43–44

@rampitec, @arsenm, would you please take a look to this 'typography'? I moved out table comments since the table width increased significantly with AliasResult::s

nikic added a comment.Mar 5 2021, 9:13 AM

This looks fine to me, though I'm not clear on how making it an enum class will help with moving to a class. Do you happen to have the patch for that already?

llvm/include/llvm/Analysis/AliasAnalysis.h
77

This paragraph should be dropped.

llvm/lib/Target/Hexagon/HexagonStoreWidening.cpp
183

Could also write this as AA->isNoAlias(L, SL) (same in some following occurrences). No strong preference though.

No, I don't have a patch at the moment, but will upload one since you're generally ok with this step.

I thought to convert the enum to a something like

class AliasResult {
public:
  int16_t Offset : 14;
  uint8_t EnumStorage : 2;
  ...
public:
  enum {...};
  ...
}

So after that access to the enum values needs AliasResult:: and I decided to split this NFC part away.
And using scoped enum allowed to find all uses.

bmahjour removed a subscriber: bmahjour.Mar 5 2021, 9:58 AM
rampitec added inline comments.Mar 5 2021, 11:06 AM
llvm/lib/Target/AMDGPU/AMDGPUAliasAnalysis.cpp
43–44

Does it work with "using AliasResult;"?

dfukalov added inline comments.Mar 6 2021, 7:42 AM
llvm/lib/Target/AMDGPU/AMDGPUAliasAnalysis.cpp
43–44

No, using AliasResult::NoAlias; in a block will be available since C++20.

At the moment I can place using AR = AliasResult; and then use AR:: but it will increase current table width of 120 to 138.

rampitec added inline comments.Mar 6 2021, 12:27 PM
llvm/lib/Target/AMDGPU/AMDGPUAliasAnalysis.cpp
43–44

Then I can only think about defines before the block and undefs after.

dfukalov updated this revision to Diff 329681.Mar 10 2021, 8:48 AM
dfukalov retitled this revision from [NFC][AA] Convert AliasResult to scoped enumeration. to [NFC][AA] Prepare to convert AliasResult to class with PartialAlias offset..

Made the enum unscoped back since it going to be unscoped within a class planned
to be new AliasResult.

dfukalov updated this revision to Diff 329701.Mar 10 2021, 10:05 AM
dfukalov marked an inline comment as done.

Converted all AliasAnalysis::alias() == AliasResult::NoAlias to
AliasAnalysis::isNoAlias().

dfukalov added inline comments.Mar 10 2021, 10:08 AM
llvm/include/llvm/Analysis/AliasAnalysis.h
77

I declined to make the enum scoped since it's going to be unscoped in a planned AliasResult class.

llvm/lib/Target/AMDGPU/AMDGPUAliasAnalysis.cpp
50–65

@rampitec, I moved the table into the function additionally.
Could you please check defines naming?

rampitec added inline comments.Mar 10 2021, 10:10 AM
llvm/lib/Target/AMDGPU/AMDGPUAliasAnalysis.cpp
50–65

Thanks, I am OK with this.

This change looks fine. Could you update the follow-up as well as a dependent patch?

asbirlea accepted this revision.Apr 8 2021, 10:33 PM
This revision is now accepted and ready to land.Apr 8 2021, 10:33 PM
This revision was landed with ongoing or failed builds.Apr 9 2021, 2:54 AM
This revision was automatically updated to reflect the committed changes.