This is an archive of the discontinued LLVM Phabricator instance.

[NFC][PowerPC] Format and clean the logic for setOperationActions to make it easier to maintain
Needs ReviewPublic

Authored by steven.zhang on Jan 6 2021, 7:17 PM.

Details

Reviewers
nemanjai
jsji
Group Reviewers
Restricted Project
Summary

We are adding more and more operation actions for different sub-features, and they are scatter over different places without clean logic.

if (hasVSX())
   ...
if (hasP9Vector())
  ...
...
if (hasVSX())
 ...

This NFC patch tried to set operation actions as following order:

By default -> hasAltivec -> hasVSX -> hasP8Altivec() -> hasP8Vector() -> hasP9Altivec() -> hasP9Vector()

So that, some operation actions could be override later and we will have clean picture on the place that setOperationAction should be added.

Diff Detail

Event Timeline

steven.zhang created this revision.Jan 6 2021, 7:17 PM
steven.zhang requested review of this revision.Jan 6 2021, 7:17 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 6 2021, 7:17 PM
steven.zhang edited the summary of this revision. (Show Details)Jan 6 2021, 7:18 PM
bsaleil added a subscriber: bsaleil.Jan 6 2021, 9:26 PM
qiucf added a subscriber: qiucf.Jan 7 2021, 12:13 AM

Maybe better to add some comments before each section? Like D78132 or X86ISelLowering.

llvm/lib/Target/PowerPC/PPCISelLowering.cpp
162

These are not guarded by any predicate, why move them?

302

Better to move them down around init_trampoline since here others are related to float?

343

So, this patch does 'overrides' like:

// Before
if (A) { setLegal(X); } else { setCustom(X); }

// After
setCustom(X); if (A) { setLegal(X); }

Looks correct. But is this good practice?

steven.zhang added inline comments.Jan 7 2021, 1:33 AM
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
162

We have other ATOMIC op later, I move them together. For BITREVERSE, I move them together with BITCAST.

330

Do we miss the case for setOperationAction(ISD::STRICT_FP_ROUND, MVT::f64, Legal); ?

343

For now, usually we have:

if (A) {
   if (B) {
     setLegal(X);
   } else {
      ...
   }
}

I am trying to flat them and move the same operation together to make it more clear. Regarding to your example, I have no good answer and that's something we can discuss. Personally, I think it makes sense as it means, by default, it is custom, if feature A enabled, it is legal. And we will have case like :

if (A) { setLegal(X); } else { setCustom(X); }
if (B) { setExpand(X);} else { setCustom(X); }

Now it is:

setCustom(X)
if (A)
   setLegal(X)
if (B)
   setExpand(X);
qiucf added inline comments.Jan 7 2021, 1:49 AM
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
330

Looks yes. It's unexpectedly guarded by P9Vector predicate. I'll check in detail and fix it later.

steven.zhang added inline comments.Jan 7 2021, 2:04 AM
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
330

Such kind of potential issue can be easily found with this re-work.

qiucf added inline comments.Jan 14 2021, 8:11 AM
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
330

Oh, this is expected: setOperationAction(ISD::FP_ROUND, MVT::f64, Legal) sets fptrunc * to double as legal. But that's illegal on Power8 or earlier subtargets. Anyway, its operation settings are redundant.

jsji resigned from this revision.Jun 2 2022, 7:43 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 2 2022, 7:43 AM