This is an archive of the discontinued LLVM Phabricator instance.

[PPC CodeGen] Fix the bitreverse.i64 intrinsic.
ClosedPublic

Authored by MaskRay on Oct 22 2017, 12:09 AM.

Details

Summary

The two 32-bit words were swapped. Update a test omitted in reverted r316270.

Event Timeline

MaskRay created this revision.Oct 22 2017, 12:09 AM
vitalybuka resigned from this revision.Oct 24 2017, 10:45 AM
jtony added inline comments.Oct 25 2017, 6:18 PM
lib/Target/PowerPC/PPCInstrInfo.td
4670

This name is outdated now. Can you go through all the logic here and make sure all the names still make sense (i.e. make sure the name represents what it does) ? To help you understand this code sequence, you can also refer the gcc generated code sequence for the C test case listed in the bug (https://bugs.llvm.org/show_bug.cgi?id=33093). And make sure you run all the LIT/LNT test cases and 3-stage bootstrap and they are all clean.

MaskRay updated this revision to Diff 120450.Oct 26 2017, 10:48 AM

Rename some def to be clearer.

% ~/Dev/llvm/build/bin/llvm-lit test/CodeGen/PowerPC/pr33093.ll test/CodeGen/PowerPC/testBitReverse.ll

  • Testing: 2 tests, 2 threads --

PASS: LLVM :: CodeGen/PowerPC/pr33093.ll (1 of 2)
PASS: LLVM :: CodeGen/PowerPC/testBitReverse.ll (2 of 2)
Testing Time: 0.51s

Expected Passes    : 2

% ~/Dev/llvm/build/bin/llvm-lit test/CodeGen/PowerPC/
Testing Time: 10.62s
Expected Passes : 819
Expected Failures : 96

@jtony I have renamed Extend Shift to Bytes0123 Bytes3012 ... because I think the new names are clearer.

32-bit __builtin_bitreverse needs a similar change and also I think there is a missing optimization:

def DWBytes0123 {

dag Word = (i32 (EXTRACT_SUBREG (RLDICL DWSwapInByte.Swap4, 32, 32), sub_32));

}

This could be msvsrld (lower doubleword) if Power ISA 3.0 is available. The original fix was my first revision and I did not expect it would take so much to time to fix a bug... I don't know much about .td but I think the`[IsISA3_0]` optimization and bitreverse32 deserve revisions of their own?

jtony edited edge metadata.Oct 26 2017, 11:22 AM

@jtony I have renamed Extend Shift to Bytes0123 Bytes3012 ... because I think the new names are clearer.

32-bit __builtin_bitreverse needs a similar change and also I think there is a missing optimization:

def DWBytes0123 {

dag Word = (i32 (EXTRACT_SUBREG (RLDICL DWSwapInByte.Swap4, 32, 32), sub_32));

}

This could be msvsrld (lower doubleword) if Power ISA 3.0 is available. The original fix was my first revision and I did not expect it would take so much to time to fix a bug... I don't know much about .td but I think the`[IsISA3_0]` optimization and bitreverse32 deserve revisions of their own?

Thanks MaskRay for rewriting the 64 bit reverse, this new patch is way clear than the last patch. Especially, I like the idea using numbers to indicate which state you are for the byte swapping. If you have more optimization for 32 bit, I would leave it to another patch. The community prefers to keep each commit small and focus on one problem. I saw you new patch still passes all the PPC IR test cases, which is good. But we want to run the whole LIT/LNT tests and bootstrap before posting it, could you also do these testing for your patch? Thank you very much for your time and effort for fixing this.

MaskRay added a comment.EditedOct 26 2017, 1:35 PM

% build/bin/llvm-lit test

Failing Tests (29): [6/46367]

  LLVM-Unit :: IR/./IRTests/CGSCCCallbacksTest.PassUtilities                                                                          
  LLVM-Unit :: IR/./IRTests/CGSCCCallbacksTest.Passes                                                                                 
  LLVM-Unit :: IR/./IRTests/FunctionCallbacksTest.AnalysisUtilities                                                                   
  LLVM-Unit :: IR/./IRTests/FunctionCallbacksTest.Passes                                                                              
  LLVM-Unit :: IR/./IRTests/LoopCallbacksTest.PassUtilities                                                                           
  LLVM-Unit :: IR/./IRTests/LoopCallbacksTest.Passes                                                                                  
  LLVM-Unit :: IR/./IRTests/ModuleCallbacksTest.AnalysisUtilities                                                                     
  LLVM-Unit :: IR/./IRTests/ModuleCallbacksTest.ParseTopLevelPipeline                                                                 
  LLVM-Unit :: IR/./IRTests/ModuleCallbacksTest.Passes                                                                                
  LLVM-Unit :: IR/./IRTests/PassManagerTest.Basic                                                                                     
  LLVM-Unit :: IR/./IRTests/PassManagerTest.CustomizedPassManagerArgs                                                                 
  LLVM-Unit :: IR/./IRTests/PassManagerTest.IndirectAnalysisInvalidation                                                              
  LLVM-Unit :: IR/./IRTests/PreservedAnalysesTest.Basic                        
  LLVM-Unit :: IR/./IRTests/PreservedAnalysesTest.PreserveSets                                                                        
  LLVM-Unit :: IR/./IRTests/PreservedAnalysisTest.Intersect                                              
  LLVM-Unit :: Transforms/Scalar/./ScalarTests/LoopPassManagerTest.Basic               
  LLVM-Unit :: Transforms/Scalar/./ScalarTests/LoopPassManagerTest.FunctionPassInvalidationOfLoopAnalyses
  LLVM-Unit :: Transforms/Scalar/./ScalarTests/LoopPassManagerTest.IndirectInvalidation         
  LLVM-Unit :: Transforms/Scalar/./ScalarTests/LoopPassManagerTest.IndirectOuterPassInvalidation
  LLVM-Unit :: Transforms/Scalar/./ScalarTests/LoopPassManagerTest.InvalidationOfBundledAnalyses
  LLVM-Unit :: Transforms/Scalar/./ScalarTests/LoopPassManagerTest.LoopChildInsertion
  LLVM-Unit :: Transforms/Scalar/./ScalarTests/LoopPassManagerTest.LoopDeletion                                                       
  LLVM-Unit :: Transforms/Scalar/./ScalarTests/LoopPassManagerTest.LoopPeerInsertion
  LLVM-Unit :: Transforms/Scalar/./ScalarTests/LoopPassManagerTest.ModulePassInvalidationOfLoopAnalyses
  LLVM-Unit :: Transforms/Utils/./UtilsTests/CloneFunc.DebugIntrinsics        
  LLVM-Unit :: Transforms/Utils/./UtilsTests/CloneFunc.InstructionOwnership
  LLVM-Unit :: Transforms/Utils/./UtilsTests/CloneFunc.NewFunctionCreated
  LLVM-Unit :: Transforms/Utils/./UtilsTests/CloneFunc.Subprogram
  LLVM-Unit :: Transforms/Utils/./UtilsTests/CloneModule.OldModuleUnchanged

Expected Passes    : 21021
Expected Failures  : 226
Unsupported Tests  : 470
Unexpected Failures: 29

% ninja check
[139/140] Running the LLVM regression tests
Testing Time: 149.61s

Expected Passes    : 21750      
Expected Failures  : 226        
Unsupported Tests  : 470
jtony accepted this revision.Oct 30 2017, 7:54 AM

LGTM.

This revision is now accepted and ready to land.Oct 30 2017, 7:54 AM
jtony added a comment.Oct 30 2017, 7:58 AM

For the Unexpected Failures: 29, do they still fail without your patch?

I reran the test just now. They did not fail.

% cat /tmp/old /tmp/new
Testing Time: 152.80s                                               
  Expected Passes    : 21750                                        
  Expected Failures  : 226                                          
  Unsupported Tests  : 470
Testing Time: 150.72s
  Expected Passes    : 21750
  Expected Failures  : 226
  Unsupported Tests  : 470
This revision was automatically updated to reflect the committed changes.